Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add the simple way to use qemu-register from other projects. #9

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

urpylka
Copy link

@urpylka urpylka commented Feb 10, 2020

Hello, please consider my PR.

  1. I updated README.md file (added more info and sample "HOW TO USE");
  2. Made cleanup register.sh;
  3. Moved codeblock from Dockerfile with user implementation to documentation.

@urpylka urpylka changed the title Add the simple way to use qemu-register from another one projects. Add the simple way to use qemu-register from other projects. Feb 10, 2020
Copy link
Member

@StefanScherer StefanScherer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@urpylka Thanks for the contribution, but I don't understand why you cannot just use the image as it is in your other projects.

I've triggered CircleCI as it hasn't automatically built your PR (which is fixed now, so if you commit/change your code it will give you feedback immediatelly).

We can't accept this PR as it is right now, as it has breaking changes, others rely on this image and how to run it.

If you want to copy the binaries from the qemu-register image to your own image, you still can do it similar as you showed in the README.md.

Maybe you can give me more insights what you want to solve with this change.


FROM busybox
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you remove the much smaller final stage?

Copy link
Author

@urpylka urpylka Feb 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was guided by idea of gap between qemu-register and user-implementation. And if you wanna build your own implementation you can use large or smaller base image (qemu-register) and later copy binary files & register.sh. It's just one docker-layer. And it can be did before (as in your solution) or later (as in mine).

You are right. busybox is smaller than debian. And if it does matter, I can fixed it in PR (return previous structure).

ADD register.sh /register.sh
CMD ["/register.sh", "--reset"]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would break the usage of this image if there is no CMD any more.

Copy link
Author

@urpylka urpylka Feb 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think that is not necessary for users, for example I can't using busybox in my project. I need debian, or alpine, or smthn else w package manager. Therefore CMD operation should using in user-implementation as I documented in README.md.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think about that a bit more and I has concluded that it can useful for tests. I return it back

@@ -1,4 +0,0 @@
#!/bin/bash
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you delete this script?
It's used in our CI pipeline which is now broken https://circleci.com/gh/hypriot/qemu-register/5

Copy link
Author

@urpylka urpylka Feb 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ooops, I didn't wan't break your CI 😅.

I removed build.sh, because I mean that construct docker build -t hypriot/qemu-register:local . (or another, written manual) better, because user must to understand what he is doing (what he is building). Also I documented that in README.md.

One moment I fix CI.

@urpylka
Copy link
Author

urpylka commented Feb 21, 2020

@StefanScherer so what about accept my PR?

Added the sample of external using. Cleanup
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants