-
Notifications
You must be signed in to change notification settings - Fork 502
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
Create kubepkg tool to building debs/rpms #935
Conversation
@justaugustus: Adding label: Reasons for blocking this PR:[Changes to certain release tools can affect our ability to test, build, and release Kubernetes. This PR must be explicitly approved by SIG Release repo admins.] Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Cool. Are we able to extend this to subprojects, e.g. cri-tools? |
@feiskyer --
Yep! You'll actually be able to this today on the debs side with: kubepkg debs --packages cri-tools --cri-tools-version 1.16.1 ...and the output is as follows: $ ls -lah bin/*
bin/nightly:
total 48M
drwxr-xr-x 2 augustus augustus 4.0K Nov 16 14:11 .
drwxr-xr-x 5 augustus augustus 4.0K Nov 15 23:31 ..
-rw-r--r-- 1 augustus augustus 11M Nov 16 14:09 cri-tools_1.16.1-0_amd64.deb
-rw-r--r-- 1 augustus augustus 9.2M Nov 16 14:10 cri-tools_1.16.1-0_arm64.deb
-rw-r--r-- 1 augustus augustus 9.2M Nov 16 14:09 cri-tools_1.16.1-0_armhf.deb
-rw-r--r-- 1 augustus augustus 9.1M Nov 16 14:10 cri-tools_1.16.1-0_ppc64el.deb
-rw-r--r-- 1 augustus augustus 9.9M Nov 16 14:11 cri-tools_1.16.1-0_s390x.deb
bin/release:
total 48M
drwxr-xr-x 2 augustus augustus 4.0K Nov 16 14:11 .
drwxr-xr-x 5 augustus augustus 4.0K Nov 15 23:31 ..
-rw-r--r-- 1 augustus augustus 11M Nov 16 14:09 cri-tools_1.16.1-0_amd64.deb
-rw-r--r-- 1 augustus augustus 9.2M Nov 16 14:10 cri-tools_1.16.1-0_arm64.deb
-rw-r--r-- 1 augustus augustus 9.2M Nov 16 14:09 cri-tools_1.16.1-0_armhf.deb
-rw-r--r-- 1 augustus augustus 9.1M Nov 16 14:10 cri-tools_1.16.1-0_ppc64el.deb
-rw-r--r-- 1 augustus augustus 9.9M Nov 16 14:11 cri-tools_1.16.1-0_s390x.deb
bin/testing:
total 48M
drwxr-xr-x 2 augustus augustus 4.0K Nov 16 14:11 .
drwxr-xr-x 5 augustus augustus 4.0K Nov 15 23:31 ..
-rw-r--r-- 1 augustus augustus 11M Nov 16 14:09 cri-tools_1.16.1-0_amd64.deb
-rw-r--r-- 1 augustus augustus 9.2M Nov 16 14:10 cri-tools_1.16.1-0_arm64.deb
-rw-r--r-- 1 augustus augustus 9.2M Nov 16 14:09 cri-tools_1.16.1-0_armhf.deb
-rw-r--r-- 1 augustus augustus 9.1M Nov 16 14:10 cri-tools_1.16.1-0_ppc64el.deb
-rw-r--r-- 1 augustus augustus 9.9M Nov 16 14:11 cri-tools_1.16.1-0_s390x.deb This is strictly because Eventually, I'd like to get to a place where this tool takes a yaml-based config with package definitions and builds the packages specified, though we won't have time to work on that piece probably until 1.18. |
4beadc4
to
4a47fbd
Compare
@justaugustus: Adding label: Reasons for blocking this PR:[Changes to certain release tools can affect our ability to test, build, and release Kubernetes. This PR must be explicitly approved by SIG Release repo admins.] Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
cmd/kubepkg/cmd/debs.go
Outdated
builds, err := kpkg.ConstructBuilds(ro.packages, ro.channels, ro.kubeVersion, ro.revision, ro.cniVersion, ro.criToolsVersion) | ||
if err != nil { | ||
log.Fatalf("err: %v", err) | ||
} |
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.
Instead do the log.Fatalf("err: %v", err)
is not better log and return the error?
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.
There should be no need to log since we fail errors with logrus.Fatal
in any case. Just returning the error should be enough to avoid logging them twice.
4a47fbd
to
ffdb456
Compare
95d284d
to
6c68c67
Compare
6c68c67
to
b7bfca3
Compare
Signed-off-by: Stephen Augustus <saugustus@vmware.com>
Signed-off-by: Stephen Augustus <saugustus@vmware.com>
Signed-off-by: Stephen Augustus <saugustus@vmware.com>
@justaugustus: Adding label: Reasons for blocking this PR:[Changes to certain release tools can affect our ability to test, build, and release Kubernetes. This PR must be explicitly approved by SIG Release repo admins.] Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Co-Authored-By: Sascha Grunert <sgrunert@suse.com> Signed-off-by: Stephen Augustus <saugustus@vmware.com>
6206087
to
7abb35e
Compare
@saschagrunert -- Thanks! I've addressed your feedback, if you have time for another sweep. :) |
@justaugustus: Adding label: Reasons for blocking this PR:[Changes to certain release tools can affect our ability to test, build, and release Kubernetes. This PR must be explicitly approved by SIG Release repo admins.] Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
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.
Good, found just another three minor nits :)
cmd/kubepkg/cmd/debs.go
Outdated
ro := rootOpts | ||
|
||
// Replace the "+" with a "-" to make it semver-compliant | ||
ro.kubeVersion = strings.TrimPrefix(ro.kubeVersion, "v") |
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.
ro.kubeVersion = strings.TrimPrefix(ro.kubeVersion, "v") | |
ro.kubeVersion = git.TrimTagPrefix(ro.kubeVersion) |
@justaugustus: Adding label: Reasons for blocking this PR:[Changes to certain release tools can affect our ability to test, build, and release Kubernetes. This PR must be explicitly approved by SIG Release repo admins.] Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
@saschagrunert -- Fixed up! |
Cool, thank you! /retest |
Co-Authored-By: Sascha Grunert <sgrunert@suse.com> Signed-off-by: Stephen Augustus <saugustus@vmware.com>
cmd/kubepkg/cmd/debs.go
Outdated
package cmd | ||
|
||
import ( | ||
"github.com/sirupsen/logrus" |
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 think we have to remove this line to fix the build error.
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.
@saschagrunert -- Yep! Just fixed it.
Signed-off-by: Stephen Augustus <saugustus@vmware.com>
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: justaugustus, saschagrunert 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 |
Refactors the debs builder into a package builder which will eventually build both debs/rpms.
This PR only attempts to convert the deb builder into a CLI tool that looks closer to how
krel
is structured and boost the test coverage a little.rpm building will be handled in a separate PR once we think a little more about how the final tool should be shaped.
For reviewers: I'm explicitly punting on testing the following functions/methods, as they will likely be massively refactored or removed in a follow-up (and this code is not currently is use):
ConstructBuilds
WalkBuilds
buildPackage
(c cfg) run()
ref: #918
cc: @kubernetes/release-engineering