[krel] gcbmgr: Dynamically retrieve build version from CI markers #1114

justaugustus · 2020-02-19T17:32:40Z

What type of PR is this?
/kind feature cleanup

What this PR does / why we need it:

pkg/release: Add getters for tools GitHub url, org, repo, and branch
pkg/release: Refactor version marker getters
[krel] gcbmgr: Dynamically retrieve build version from CI markers
pkg/util: Check HTTP status code before return in GetURLResponse()
pkg/release: Allow version getters to return [non-]SemVer-compliant

Signed-off-by: Stephen Augustus saugustus@vmware.com

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

[krel] gcbmgr: Dynamically retrieve build version from CI markers

hasheddan

@justaugustus this is a nice improvement here 👍 could we add some comments to each of the exported funcs as well?

Signed-off-by: Stephen Augustus <saugustus@vmware.com>

k8s-ci-robot · 2020-02-19T20:20:55Z

@justaugustus: Adding label: do-not-merge/blocked-paths because PR changes a protected file.

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.

Signed-off-by: Stephen Augustus <saugustus@vmware.com>

justaugustus · 2020-02-20T00:21:18Z

/assign @hasheddan @saschagrunert @hoegaarden @tpepper
/priority important-soon

justaugustus · 2020-02-20T00:23:20Z

@justaugustus this is a nice improvement here could we add some comments to each of the exported funcs as well?

Thanks! I've moved that portion of the PR to #1115, which has already merged.

hasheddan

This is looking good @justaugustus :) One small change proposed that I would like to get your thoughts on 👍

hasheddan · 2020-02-20T04:58:38Z

pkg/release/release.go

+// GetToolRepo checks if the 'TOOL_REPO' environment variable is set.
+// If 'TOOL_REPO' is non-empty, it returns the value. Otherwise, it returns DefaultToolRepo.
+func GetToolRepo() string {
+	toolRepo := os.Getenv("TOOL_REPO")


I generally try to steer away from using env variables if possible. However, if we do want to use them, I would prefer for it to be exposed at the command level and then either checked, or passed as a parameter to a library function like this. The latter case could look something like this in gcbmgr or any other command:

toolRepo := release.GetToolRepo(os.Getenv("TOOL_REPO"))

How would you feel about that?

Big +1 from me about steering awat from using env vars directly in functions. The good advice I read some time ago is to use the env vars just in the main function and then pass them around as function parameters.

@hasheddan -- I would prefer to keep this one as-is.
The idea being that we want to make it a little difficult to run the tool in a non-k/release@master manner.

If you used some flags, it's possible maybe you copy/pasted something.
If you set three environment variables, there's some intent behind overriding the default tools org, repo, and branch.

I definitely see your point, but I could also see someone copy pasting setting env vars just like the command with flags. In addition, a flag must always be set explicitly on each command run, while an env var could be lurking from a previous operation in the same session. All of that being said, I think this is mostly a stylistic preference (rather than a right / wrong thing) so I am comfortable moving forward if you feel strongly :)

The case is, it's not about making it harder for people to not use the flags itself, but it's much easier to maintain the thing in the future if logic related to env-vars is stored in the main/root place. These are just good practices and makes code more futureproof.

@bartsmykla -- That's essentially what's being done here.
By putting the logic for these env vars in the release package, instead of the command, I'm trying to ensure that anyone who needs to use them has a consistent experience.

As a note, this is one of the first few passes on this tool rewrite.
In this first phase, I'm primarily concerned w/ replicating the logic of the existing tool, which uses env vars:

release/gcbmgr

Lines 152 to 154 in c13fa28

readonly TOOL_ORG="${TOOL_ORG:-kubernetes}"

readonly TOOL_REPO="${TOOL_REPO:-release}"

readonly TOOL_BRANCH="${TOOL_BRANCH:-master}"

There are some pieces that no longer make sense to carry over, but this isn't one of them just yet.

hasheddan · 2020-02-20T04:59:21Z

pkg/release/release.go

 // BuiltWithBazel determines whether the most recent release was built with Bazel.
-func BuiltWithBazel(path, releaseKind string) (bool, error) {
+func BuiltWithBazel(workDir, releaseKind string) (bool, error) {


Thank you for these updates, much better param name :)

Signed-off-by: Stephen Augustus <saugustus@vmware.com>

justaugustus · 2020-02-20T05:19:43Z

Added these two bits:

pkg/util: Check HTTP status code before return in GetURLResponse()
pkg/release: Allow version getters to return [non-]SemVer-compliant

bartsmykla · 2020-02-20T05:10:19Z

pkg/release/release.go

+
+func GetLatestCIKubeVersion() (string, error) {
+	logrus.Info("Retrieving Kubernetes latest build version...")
+	return GetKubeVersion("https://dl.k8s.io/ci/latest.txt")


We have three different urls here. I think it will be better to put them as const at the beginning of the file. It will be more future proof if the links will be used somewhere else, and also more consise.

bartsmykla · 2020-02-20T05:10:54Z

pkg/release/release.go

+	versionMarkerFile := fmt.Sprintf("%s.txt", versionMarker)
+	logrus.Infof("Version marker file: %s", versionMarkerFile)
+
+	u, parseErr := url.Parse("https://dl.k8s.io/ci")


The same comment with putting the url at the top of the file applies here too.

@bartsmykla -- Hehe. I was actually planning on doing that for a follow-up.
I want to think a little more about version markers before parameterizing them.

bartsmykla · 2020-02-20T05:17:05Z

pkg/release/release.go

+// GetToolRepo checks if the 'TOOL_REPO' environment variable is set.
+// If 'TOOL_REPO' is non-empty, it returns the value. Otherwise, it returns DefaultToolRepo.
+func GetToolRepo() string {
+	toolRepo := os.Getenv("TOOL_REPO")


Big +1 from me about steering awat from using env vars directly in functions. The good advice I read some time ago is to use the env vars just in the main function and then pass them around as function parameters.

hasheddan

/lgtm

k8s-ci-robot · 2020-02-20T05:50:18Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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:

~~cmd/krel/OWNERS~~ [hasheddan,justaugustus]
~~pkg/OWNERS~~ [hasheddan,justaugustus]

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

k8s-ci-robot requested review from hasheddan and saschagrunert February 19, 2020 17:34

k8s-ci-robot added area/release-eng Issues or PRs related to the Release Engineering subproject sig/release Categorizes an issue or PR as relevant to SIG Release. labels Feb 19, 2020

hasheddan reviewed Feb 19, 2020

View reviewed changes

justaugustus changed the title ~~[WIP] pkg/git: Add getters for GitHub URLs~~ [WIP] [krel] gcbmgr: Add additional checks Feb 19, 2020

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 19, 2020

pkg/release: Add getters for tools GitHub url, org, repo, and branch

8c0cec7

Signed-off-by: Stephen Augustus <saugustus@vmware.com>

justaugustus force-pushed the krel-gcbmgr branch from 3210d48 to 8c0cec7 Compare February 19, 2020 20:20

justaugustus force-pushed the krel-gcbmgr branch 2 times, most recently from 48780f7 to 97c2806 Compare February 20, 2020 00:12

justaugustus added 2 commits February 19, 2020 19:16

pkg/release: Refactor version marker getters

3e52346

Signed-off-by: Stephen Augustus <saugustus@vmware.com>

[krel] gcbmgr: Dynamically retrieve build version from CI markers

f2cc7cd

Signed-off-by: Stephen Augustus <saugustus@vmware.com>

justaugustus force-pushed the krel-gcbmgr branch from 97c2806 to f2cc7cd Compare February 20, 2020 00:17

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Feb 20, 2020

k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. and removed needs-kind Indicates a PR lacks a `kind/foo` label and requires one. labels Feb 20, 2020

justaugustus changed the title ~~[WIP] [krel] gcbmgr: Add additional checks~~ [krel] gcbmgr: Dynamically retrieve build version from CI markers Feb 20, 2020

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 20, 2020

k8s-ci-robot assigned hasheddan, hoegaarden, saschagrunert and tpepper Feb 20, 2020

k8s-ci-robot added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. and removed needs-priority labels Feb 20, 2020

hasheddan reviewed Feb 20, 2020

View reviewed changes

justaugustus added 2 commits February 20, 2020 00:15

pkg/util: Check HTTP status code before return in GetURLResponse()

812201d

Signed-off-by: Stephen Augustus <saugustus@vmware.com>

pkg/release: Allow version getters to return [non-]SemVer-compliant

497d958

Signed-off-by: Stephen Augustus <saugustus@vmware.com>

bartsmykla reviewed Feb 20, 2020

View reviewed changes

justaugustus removed the do-not-merge/blocked-paths Indicates that a PR should not merge because it touches files in blocked paths. label Feb 20, 2020

hasheddan approved these changes Feb 20, 2020

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 20, 2020

k8s-ci-robot merged commit 11f0f14 into kubernetes:master Feb 20, 2020

k8s-ci-robot added this to the v1.18 milestone Feb 20, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[krel] gcbmgr: Dynamically retrieve build version from CI markers #1114

[krel] gcbmgr: Dynamically retrieve build version from CI markers #1114

justaugustus commented Feb 19, 2020 •

edited

Loading

hasheddan left a comment

k8s-ci-robot commented Feb 19, 2020

Reasons for blocking this PR:

justaugustus commented Feb 20, 2020

justaugustus commented Feb 20, 2020

hasheddan left a comment

hasheddan Feb 20, 2020

bartsmykla Feb 20, 2020

justaugustus Feb 20, 2020 •

edited

Loading

hasheddan Feb 20, 2020

bartsmykla Feb 20, 2020

justaugustus Feb 20, 2020

hasheddan Feb 20, 2020

justaugustus commented Feb 20, 2020

bartsmykla Feb 20, 2020

bartsmykla Feb 20, 2020

justaugustus Feb 20, 2020

bartsmykla Feb 20, 2020

hasheddan left a comment

k8s-ci-robot commented Feb 20, 2020

	readonly TOOL_ORG="${TOOL_ORG:-kubernetes}"
	readonly TOOL_REPO="${TOOL_REPO:-release}"
	readonly TOOL_BRANCH="${TOOL_BRANCH:-master}"

[krel] gcbmgr: Dynamically retrieve build version from CI markers #1114

[krel] gcbmgr: Dynamically retrieve build version from CI markers #1114

Conversation

justaugustus commented Feb 19, 2020 • edited Loading

hasheddan left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Feb 19, 2020

Reasons for blocking this PR:

justaugustus commented Feb 20, 2020

justaugustus commented Feb 20, 2020

hasheddan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justaugustus Feb 20, 2020 • edited Loading

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 Feb 20, 2020

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

k8s-ci-robot commented Feb 20, 2020

justaugustus commented Feb 19, 2020 •

edited

Loading

justaugustus Feb 20, 2020 •

edited

Loading