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

make Dockerfile two-staged #472

Merged
merged 1 commit into from
Mar 30, 2021
Merged

Conversation

StefanSchoof
Copy link
Contributor

run in user context
add foreground option

@r00t-
Copy link
Contributor

r00t- commented Mar 29, 2021

nice things we can have with --foreground 🎉
(previously one could not run vzlogger inside docker in any sane way, as docker requires the process to stay in foreground.)
(the second stage makes it so that the tools required for compilation don't become part of the image, making for a smaller image. - once #402 is fixed we could switch to alpine for a really small image.)

only thing i would have preferred is to not re-format the unchanged lines, as this now replaces the whole file.

@mbehr1: should we test the Dockerfile as part of the cI checks? can we do that conditionally only if it was changed? or maybe just replace the build with the dockerfile build? questions, questions...

Dockerfile Show resolved Hide resolved
@r00t- r00t- changed the title make Dockerfile two stages make Dockerfile two-staged Mar 29, 2021
@StefanSchoof
Copy link
Contributor Author

About the unchanged lines: I know what you mean. Since I removed the sudo and subversion package and extra spaces and the line endings, there are only few unchanged lines. And this format will make changes to packages cleaner. But if you like I can format the apt line as previous.

@r00t-
Copy link
Contributor

r00t- commented Mar 29, 2021

imho good to merge,
just i would prefer if i wasn't the only one reviewing stuff.
and all other vzlogger people seem to have little time.

@r00t-
Copy link
Contributor

r00t- commented Mar 29, 2021

@StefanSchoof

Since I removed the sudo and subversion package

actually that's probably the reason for noting this, btw.
i stared at the diff for a minute, and could not really tell what was changed.
the best way would probably to first make a commit that only changes the formatting,
and then put the actual change on top of that.
but it's not necessary here.

@m-reuter
Copy link
Contributor

Maybe makes sense to link everything static , also the other dependencies. That way the image will stay smaller as no libs need to be installed.

@StefanSchoof
Copy link
Contributor Author

I think a complete static version would be great. But this needs work from a person with more knowledge of the c build chain.

Dockerfile Outdated Show resolved Hide resolved
run in user context
add foreground option

Add comment about static libsml

fix grammer in comment
@r00t-
Copy link
Contributor

r00t- commented Mar 30, 2021

I think a complete[ly] static version would be great

inside docker it doesn't make a difference,
but it would make the plain binary very portable, without needing docker

@r00t- r00t- merged commit fddf479 into volkszaehler:master Mar 30, 2021
@r00t- r00t- mentioned this pull request Apr 1, 2021
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.

3 participants