push build: initial migration to krel #941

hasheddan · 2019-11-25T05:24:31Z

Initial steps to move push-build.sh to krel push. Steps to still be completed include:

Implement remainder of push logic (do we feel the need to use bazel to push the build? If not then we can likely do it in a cleaner fashion using gcloud sdk rather than executing bazel using os.Exec.)
Add unit tests for new pkg functions

Please feel free to review and give any immediate thoughts or feedback!

/cc @saschagrunert @justaugustus

Signed-off-by: hasheddan <[email protected]>

saschagrunert

I love it! I just have a couple of comments and nits 👇

saschagrunert · 2019-11-26T08:14:34Z

pkg/util/common.go

@@ -104,7 +108,7 @@ func FakeGOPATH(srcDir string) (string, error) {
 	log.Printf("GOPATH: %s", os.Getenv("GOPATH"))

 	gitRoot := fmt.Sprintf("%s/src/k8s.io", baseDir)
-	if err := os.MkdirAll(gitRoot, 0o755); err != nil {
+	if err := os.MkdirAll(gitRoot, os.FileMode(int(0755))); err != nil {


Hm, what is the benefit of doing it like this? 🙃

I was getting a compiler error on my local, but I think my config was just wrong. I'll change back :)

Okay, maybe you have no go 1.13 on your local machine?

saschagrunert · 2019-11-26T08:18:11Z

pkg/util/common.go

@@ -122,3 +126,47 @@ func FakeGOPATH(srcDir string) (string, error) {

 	return gitRoot, nil
 }
+
+// UntarAndRead opens a tarball and reads contents of a file inside.
+func UntarAndRead(tarPath, filePath string) (string, error) {


Do we wanna write some tests for this function and MoreRecent?

Definitely! In my check list above

nits:

Do we maybe want to return the io.Reader instead of a string? That way it

would be more ideomatic

a caller could wrap a LimitedReader around it or do whatever

Is a name like ReadFileFromGzippedTar(...) a "truer" name. Would it be a better name? Not really ... 🤔#namingThings

Agree to both of these points :) This structure was mostly a result of trying to mimic the exact behavior of

release/push-build.sh

Line 131 in 471139d

LATEST=$(tar -O -xzf $KUBE_ROOT/_output/release-tars/$FLAGS_release_kind.tar.gz $FLAGS_release_kind/version)

saschagrunert · 2019-11-26T08:20:44Z

pkg/release/release.go

+
+// BuiltWithBazel determines whether the most recent release was built with Bazel.
+func BuiltWithBazel(path string, releaseKind string) (bool, error) {
+	bazelBuild := path + bazelBuildPath + releaseKind + tarballExtension


I suggest using filepath.Join for concatenating paths.

saschagrunert · 2019-11-26T08:23:48Z

cmd/krel/cmd/push.go

+	}
+
+	if isBazel {
+		log.Println("Using Bazel build version.")


Its a nitty nit, but we could remove the dot at the end of a log to have a uniform logging layout. :)

saschagrunert · 2019-11-26T08:24:58Z

pkg/release/release.go

+}
+
+// IsValidReleaseBuild checks if build version is valid for release.
+func IsValidReleaseBuild(build string) (bool, error) {


I'd suggest in returning either a bool or an error here to make its usage simpler.

I was initially thinking that as well, but I didn't know if there would be a use case for this function in the future where it might be valid to know if there was not a match or if the comparison failed altogether. If we don't think this is necessary, I think a good fix would be to use regexp.MustCompile in init() then just returning MatchString() here!

Hm I see your point, so I guess we can also keep the current approach. :)

hasheddan

Thanks for the review @saschagrunert! Any thoughts on the bazel question in the PR body?

saschagrunert · 2019-11-27T06:18:57Z

Thanks for the review @saschagrunert! Any thoughts on the bazel question in the PR body?

You're welcome. No, I'm definitely not sure if we need the bazel build in first place. 🤷‍♂️

justaugustus · 2019-11-27T09:04:27Z

Thanks for the review @saschagrunert! Any thoughts on the bazel question in the PR body?

You're welcome. No, I'm definitely not sure if we need the bazel build in first place.

@saschagrunert @hasheddan -- We'll need to support bazel and non-bazel builds.
Can you open up an issue to discuss that?

--

I'll come around for a review sweep on this once you take care of Sascha's initial feedback.

hasheddan · 2019-11-27T17:30:05Z

@justaugustus @saschagrunert definitely agree that we want to support both bazel and non-bazel builds (both are in this current iteration), but do we need to use bazel to actually push the build?

References:

release/push-build.sh

Line 198 in 43e73cc

release::gcs::bazel_push_build $GCS_DEST $LATEST $KUBE_ROOT/_output \
release/lib/releaselib.sh

Line 1117 in 43e73cc

bazel run //:push-build $gcs_stage $gcs_destination || return 1
https://github.com/kubernetes/kubernetes/blob/7c540ecdac946edfefe03bba50d1dd08ec4b62d5/build/root/BUILD.root#L39

justaugustus · 2019-12-04T09:36:30Z

@hasheddan -- Mentioned on the SIG Release call this week, but also dropping here...
Yes, I think it's fine for us to proceed with a non-bazel push for both scenarios.

(Sending this for a retest to see if our newly-enabled linters pick up something.)
/retest

justaugustus · 2019-12-04T09:38:02Z

/retest

justaugustus · 2019-12-04T09:39:12Z

(Apparently the /retest behavior may have changed?)
/test all

justaugustus · 2019-12-04T09:40:25Z

/priority important-longterm
/kind feature

hoegaarden · 2019-12-04T11:11:25Z

cmd/krel/cmd/push.go

+	// Check if latest build uses bazel
+	dir, err := os.Getwd()
+	if err != nil {
+		log.Fatal(err)


Should we let the errors bubble up instead of exiting via log.Fatal here (and at the other places)?

Do you mean that we should keep executing? I think it depends on the error. This one I think we should exit immediately because we will be unable to reliably push a build if we cannot get the working dir.

What I meant is:

return an error here instead of log.Fatal(...)

this error should bubble up all the way to be either handled or results in an os.Exit(...) or such somewhere centrally.

I would not expect a function that returns an error also to shut down the whole process. If nothing else, that is IMHO not really intuitive and ~~hard~~ stupid to test.

hoegaarden · 2019-12-04T11:35:17Z

pkg/util/common.go

+
+		if h.Name == filePath {
+			file, err := ioutil.ReadAll(tr)
+			return strings.TrimSuffix(string(file), "\n"), err


Is this (trimming of a newline) a general thing we need to do when extracting a file from a tarball, or is this specific to our case (reading that version file)?

This is more of a specific use-case thing, which, if this function is made more general as suggested above, should probably be deferred to the user of the function (i.e. the push build cmd). Thanks for pointing this out!

hoegaarden · 2019-12-04T11:44:04Z

pkg/util/common.go

@@ -122,3 +126,47 @@ func FakeGOPATH(srcDir string) (string, error) {

 	return gitRoot, nil
 }
+
+// UntarAndRead opens a tarball and reads contents of a file inside.
+func UntarAndRead(tarPath, filePath string) (string, error) {


nits:

Do we maybe want to return the io.Reader instead of a string? That way it

would be more ideomatic

a caller could wrap a LimitedReader around it or do whatever

Is a name like ReadFileFromGzippedTar(...) a "truer" name. Would it be a better name? Not really ... 🤔#namingThings

justaugustus · 2019-12-11T17:09:19Z

Let's get this in.
@hasheddan -- please address PR feedback in a follow-up.

/lgtm
/approve

cpanato · 2019-12-11T17:13:36Z

/lgtm

k8s-ci-robot · 2019-12-11T17:13:53Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cpanato, hasheddan, justaugustus

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:

~~OWNERS~~ [justaugustus]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

saschagrunert · 2019-12-11T17:30:25Z

Awesome, it is great to see that this is merged!

hasheddan · 2019-12-11T17:46:42Z

@justaugustus I'll get the follow-up PR in ASAP, thanks! :)

push build: initial migration to krel

b23e152

Signed-off-by: hasheddan <[email protected]>

k8s-ci-robot requested review from justaugustus and saschagrunert November 25, 2019 05:24

saschagrunert reviewed Nov 26, 2019

View reviewed changes

hasheddan commented Nov 26, 2019

View reviewed changes

saschagrunert mentioned this pull request Nov 27, 2019

[krel] Refactor existing bash scripts into go tools #918

Closed

16 tasks

justaugustus added this to the v1.18 milestone Dec 4, 2019

k8s-ci-robot added priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. kind/feature Categorizes issue or PR as related to a new feature. labels Dec 4, 2019

hoegaarden reviewed Dec 4, 2019

View reviewed changes

justaugustus mentioned this pull request Dec 9, 2019

move push-build to kubernetes/kubernetes #829

Closed

k8s-ci-robot assigned justaugustus Dec 11, 2019

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Dec 11, 2019

k8s-ci-robot assigned cpanato Dec 11, 2019

k8s-ci-robot merged commit e7cfb62 into kubernetes:master Dec 11, 2019

hasheddan mentioned this pull request Dec 12, 2019

krelpush: logging and tests #972

Merged

saschagrunert mentioned this pull request Aug 4, 2020

[krel] Finish-up krel push #1459

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

push build: initial migration to krel #941

push build: initial migration to krel #941

hasheddan commented Nov 25, 2019

saschagrunert left a comment

saschagrunert Nov 26, 2019

hasheddan Nov 26, 2019

saschagrunert Nov 27, 2019

saschagrunert Nov 26, 2019

hasheddan Nov 26, 2019

hoegaarden Dec 4, 2019

hasheddan Dec 7, 2019

saschagrunert Nov 26, 2019

saschagrunert Nov 26, 2019

saschagrunert Nov 26, 2019

hasheddan Nov 26, 2019

saschagrunert Nov 27, 2019

hasheddan left a comment

saschagrunert commented Nov 27, 2019 •

edited

Loading

justaugustus commented Nov 27, 2019

hasheddan commented Nov 27, 2019

justaugustus commented Dec 4, 2019

justaugustus commented Dec 4, 2019

justaugustus commented Dec 4, 2019

justaugustus commented Dec 4, 2019

hoegaarden Dec 4, 2019

hasheddan Dec 7, 2019

hoegaarden Dec 9, 2019

hoegaarden Dec 4, 2019

hasheddan Dec 7, 2019

hoegaarden Dec 4, 2019

justaugustus commented Dec 11, 2019

cpanato commented Dec 11, 2019

k8s-ci-robot commented Dec 11, 2019

saschagrunert commented Dec 11, 2019

hasheddan commented Dec 11, 2019

push build: initial migration to krel #941

push build: initial migration to krel #941

Conversation

hasheddan commented Nov 25, 2019

saschagrunert left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hasheddan left a comment

Choose a reason for hiding this comment

saschagrunert commented Nov 27, 2019 • edited Loading

justaugustus commented Nov 27, 2019

hasheddan commented Nov 27, 2019

justaugustus commented Dec 4, 2019

justaugustus commented Dec 4, 2019

justaugustus commented Dec 4, 2019

justaugustus commented Dec 4, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justaugustus commented Dec 11, 2019

cpanato commented Dec 11, 2019

k8s-ci-robot commented Dec 11, 2019

saschagrunert commented Dec 11, 2019

hasheddan commented Dec 11, 2019

saschagrunert commented Nov 27, 2019 •

edited

Loading