-
Notifications
You must be signed in to change notification settings - Fork 505
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
Update debian/ubuntu build definitions #720
Conversation
/hold when we say EOL, what are we referring to here? EOL according to this project's policies, or the distro's policies? is there a link for this decision? etc. would like someone from patch release team to review (ie: @tpepper who is assigned) and |
/assign @tpepper |
@spiffxp -- I'm thinking OS EOL here. The addition of bionic to the list (#719) was the inspiration for this. |
Updated this PR to include collapsing debian-like allDistros, serverDistros var into distros (allows building all artifacts for all supported debian-like distros). /assign @sumitranr |
In general I think I'm ok with this, but I really need to see commit messages. I'm inferring from the three commits that they do something sensible to me, but even with the context in this PR's conversation I can't validate what I'm reviewing versus the author's intent. Please amend the three commits with a commit message body as per https://chris.beams.io/posts/git-commit/ |
/hold |
@tpepper -- updated the commit messages! |
As far as documenting a policy or whatever, I'm not sure if this repo or someplace else is more appropriate. As a start, can you leave a comment to this effect next to the distro list, to explain why the default value is what it is? |
Looks good to me, but I want to hold this until after 1.13.7 and 1.14.3 next week, which are loaded with content. But then I'd also like to see if we can even get in more of this type of cleaning in June though ahead of 1.15. |
/retest |
Several build definitions for unsupported versions of debian and ubuntu remained in the repo; this commit cleans them up. "Unsupported" in this context refers to versions which fall outside of the distros support policy. Signed-off-by: Stephen Augustus <[email protected]>
Prior to this commit, two variables (allDistros, serverDistros) existed for defining the distros on which certain binaries should be built. As there is no delineation between "server" and "non-server" distros for the binaries we build, there is no need for separate variables to describe them. This commit collapses the variables into a single `distro` variable. Signed-off-by: Stephen Augustus <[email protected]>
We currently use a single build definition (for bionic) and symlink folders for each debian-like distro to it. Previously, several of these build definitions only contained a symlink for the kubectl binary. This commit updates the symlinks so that all kubernetes binaries are built for all debian-like distros that we have defined. Signed-off-by: Stephen Augustus <[email protected]>
@tpepper -- SGTM. @spiffxp -- Added a comment about the Lines 71 to 74 in 66ff86d
|
@@ -1 +0,0 @@ | |||
../bionic/kubectl |
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.
Can you clarify why this is deleted?
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.
@sumitranr -- previously, only the kubectl definitions were symlinked. This PR symlinks the full directory, so all binaries are built for each deb distro flavor.
See here: https://github.com/kubernetes/release/pull/720/files#diff-e3f51e32c56fc68e9e81fd19c758c774
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.
ah! i should have read your change description more thoroughly. you have already described it there. thanks for clarifying.
/lgtm thanks for fixing the script. |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: justaugustus, sumitranr The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
@tpepper -- turns out we'll need this merged before the 1.13.7/1.14.3 cuts. I spoke with @sumitranr and he had to make similar changes to what's in the PR locally to be able to publish 1.12.9. |
Ok if we merge this today, could you run (@sumitranr) a test build today or Monday just to triple check the output debs look 100% as expected? |
yes. i will double check and do a test build |
/hold cancel |
distro
variableSigned-off-by: Stephen Augustus [email protected]