-
Notifications
You must be signed in to change notification settings - Fork 18
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
build: Add debian based server image build #193
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comments on 1st patch only.
@@ -31,26 +31,34 @@ install_sambacc() { | |||
|
|||
|
|||
local action=install-from-copr-repo | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
extra whitespace
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gone
elif [ "${#wheels[@]}" -eq 1 ]; then | ||
fi | ||
if [ "${#wheels[@]}" -eq 1 ]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why the change from an elif
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it was unintentional, part of a bigger change that somehow did not make it into the PR but should be back now.
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
extra whitespace
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gone
action=install-from-repo | ||
fi | ||
echo "no local sambacc installation source found. falling back to copr install." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this line belongs there. It will pretty much always print out, even in cases where it not true.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Originally, I had implemented a changed/improved logic for a preference order of installation sources that would only trigger this message when it was actually true.
Somehow parts of that change got dropped from the patch in this PR.
Now I added it back.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
now this is in #195
cf787c7
to
ef45ee2
Compare
I am thinking of maybe splitting the first commit out into a separate PR and adding debian flavor for more image builds |
this is now based on #195 |
ef45ee2
to
f75a4c8
Compare
So, I'm going to be honest and say that I'm not super enthusiastic about adding a new distro image - especially one that no one external to the project has requested. This does not mean I'm firmly opposed, but it seems like a future maintenance burden with little to no immediate payoff. Plus, I find that we already have enough variations that things are complicated to wrangle. We have the fedora builds which are tested and released. On top of all those we have the various nightly and dev-build subvariants. I would rather get a stronger handle on the stuff we're already doing, including getting the arm64 builds included in the regular build process, and then getting the centos variants part of the release process before adding debian --- UNLESS there's some big demand that I'm unaware of. |
This pull request now has conflicts with the target branch. Please resolve these conflicts and force push the updated branch. |
this triggers the debian based server build: make KIND=server OS_NAME=debian build-image Signed-off-by: Michael Adam <[email protected]>
f75a4c8
to
c333630
Compare
this adds debian based server image build flavor.
trigger with
make KIND=server OS_NAME=debian build-image