move references from TOOL_BRANCH to be TOOL_REF #1906

cpanato · 2021-02-11T14:02:43Z

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

move references from TOOL_BRANCH to be TOOL_REF

to be honest I'm not 100% sure if this is enough, opening the PR for feedback

/assign @saschagrunert @hasheddan @puerco

Which issue(s) this PR fixes:

Fixes #1028

Special notes for your reviewer:

Does this PR introduce a user-facing change?

move references from TOOL_BRANCH to be TOOL_REF

puerco

I think we still need to add a bit of plumbing here to checkout a specific commit if we want to use a specific ref instead of a branch, let me know what you think:

puerco · 2021-02-11T19:22:39Z

gcb/release/cloudbuild.yaml

@@ -21,7 +21,7 @@ steps:
  dir: "go/src/k8s.io"
  args:
  - "clone"
-  - "--branch=${_TOOL_BRANCH}"
+  - "--branch=${_TOOL_REF}"


As it stands we will successfully get the value from the substitution here. But I think that this still leaves us only with the possibility to use a specific branch (or a tag). We still need to check out a commit if we want to use an arbitrary ref.

puerco · 2021-02-11T19:24:13Z

gcb/release/cloudbuild.yaml

@@ -21,7 +21,7 @@ steps:
  dir: "go/src/k8s.io"
  args:
  - "clone"
-  - "--branch=${_TOOL_BRANCH}"
+  - "--branch=${_TOOL_REF}"
  - "https://github.com/${_TOOL_ORG}/${_TOOL_REPO}"



One possibility to checkout a ref would be to insert a new GCB stage here that simply does git checkout SHA when TOOL_REF is a string that looks like a sha or sha+branch slug. It should be fairly quick as the git container is already cached and is one of the official GCB builders anyway.

The other option would be to change the clone stage, modify the entry point to sh/bash, and call git twice from the shell (clone and then checkout).

thanks for the feedback, I was unsure about that, I will ping you in slack to discuss if you can offcourse

cpanato · 2021-02-19T11:30:18Z

when have some free cycles can you take a look at this change? thanks!
@saschagrunert @puerco

puerco

In general, it looks good to me. Renaming git.RevParse() touches a lot of things but I'm confident you got them all covered. From my perspective it should work, I would vote to merge it after 1.21 beta.0 and give it a few test runs.

pkg/git/git.go

puerco · 2021-02-23T07:01:04Z

pkg/git/git.go

+func (r *Repo) RevParse(rev string) (string, error) {
+	// Try to resolve the rev
+	ref, err := r.inner.ResolveRevision(plumbing.Revision(rev))
+	if err != nil {
+		return "", err
+	}
+
+	return ref.String(), nil
+}


I'm amazed at how much work you put in to rename the old git.RevParse(). I think I would have added the functionality to the same function ;)

saschagrunert

Code LGTM beside the single nit. @cpanato can you give it some manual test runs?

pkg/release/repository.go

pkg/git/git.go

k8s-ci-robot · 2021-03-13T15:26:48Z

@cpanato: The following tests failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
pull-release-image-k8s-ci-builder	`a0b09b4`	link	`/test pull-release-image-k8s-ci-builder`
pull-release-image-releng-ci	`a0b09b4`	link	`/test pull-release-image-releng-ci`
pull-release-image-kubepkg	`a0b09b4`	link	`/test pull-release-image-kubepkg`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

cpanato · 2021-03-21T09:42:56Z

using:
TOOL_ORG=cpanato
TOOL_REF=91a1579eb3161d6c805a77e01e89893baa3a9885

from here: cpanato@91a1579

stage build: https://console.cloud.google.com/cloud-build/builds/f3ede851-254b-4ad7-bd26-2aa3349ab3b3?project=kubernetes-release-test

and in the logs can see the custom change

Step #3: INFO[2021-03-21T09:41:59Z] Using CUSTOM ORG/REPO/BRANCH:
Step #3: cpanato/release/91a1579eb3161d6c805a77e01e89893baa3a9885

cc @saschagrunert @puerco

cpanato · 2021-03-21T11:58:12Z

using:
TOOL_ORG=cpanato
TOOL_REF=testing-ref

from here: https://github.com/cpanato/release/tree/testing-ref

stage build: https://console.cloud.google.com/cloud-build/builds/e312f9a4-c279-4944-b24d-71ff44ba40d6?project=kubernetes-release-test

and in the logs can see the custom change

Step #3: INFO[2021-03-21T11:57:16Z] Using CUSTOM ORG/REPO/BRANCH:
Step #3: cpanato/release/testing-ref

saschagrunert

/lgtm

k8s-ci-robot · 2021-03-22T08:26:09Z

[APPROVALNOTIFIER] This PR is APPROVED

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

~~OWNERS~~ [cpanato,saschagrunert]

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 assigned hasheddan Feb 11, 2021

k8s-ci-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Feb 11, 2021

k8s-ci-robot assigned puerco and saschagrunert Feb 11, 2021

k8s-ci-robot added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-priority labels Feb 11, 2021

k8s-ci-robot requested review from saschagrunert and sethmccombs February 11, 2021 14:02

puerco reviewed Feb 11, 2021

View reviewed changes

cpanato changed the title ~~move references from TOOL_BRANCH to be TOOL_REF~~ WIP move references from TOOL_BRANCH to be TOOL_REF Feb 17, 2021

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 17, 2021

cpanato force-pushed the GH-1028 branch 3 times, most recently from 10330de to edd198e Compare February 18, 2021 15:40

k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 19, 2021

cpanato force-pushed the GH-1028 branch from ab40cd3 to f68d1c5 Compare February 19, 2021 10:46

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

cpanato force-pushed the GH-1028 branch 2 times, most recently from fa13b34 to 1e62209 Compare February 19, 2021 11:12

cpanato changed the title ~~WIP move references from TOOL_BRANCH to be TOOL_REF~~ move references from TOOL_BRANCH to be TOOL_REF Feb 19, 2021

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 19, 2021

cpanato requested a review from puerco February 19, 2021 11:29

puerco reviewed Feb 23, 2021

View reviewed changes

cpanato force-pushed the GH-1028 branch from 1e62209 to a0b09b4 Compare February 23, 2021 09:00

saschagrunert reviewed Mar 1, 2021

View reviewed changes

pkg/release/repository.go Outdated Show resolved Hide resolved

pkg/git/git.go Show resolved Hide resolved

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

cpanato added 3 commits March 21, 2021 10:27

move references from TOOL_BRANCH to be TOOL_REF

d84ae47

mocks: update mocks

4ab2b68

git: add new function to do the plain rev parse

80a5fb4

cpanato force-pushed the GH-1028 branch from a0b09b4 to 80a5fb4 Compare March 21, 2021 09:27

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 21, 2021

update cloudbuilds

846296d

cpanato force-pushed the GH-1028 branch from 3b84427 to 846296d Compare March 21, 2021 09:45

saschagrunert approved these changes Mar 22, 2021

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 22, 2021

k8s-ci-robot merged commit ec1fb50 into kubernetes:master Mar 22, 2021

k8s-ci-robot added this to the v1.21 milestone Mar 22, 2021

cpanato deleted the GH-1028 branch March 22, 2021 08:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

move references from TOOL_BRANCH to be TOOL_REF #1906

move references from TOOL_BRANCH to be TOOL_REF #1906

cpanato commented Feb 11, 2021

puerco left a comment

puerco Feb 11, 2021

puerco Feb 11, 2021

cpanato Feb 16, 2021

cpanato commented Feb 19, 2021

puerco left a comment

puerco Feb 23, 2021

saschagrunert left a comment

k8s-ci-robot commented Mar 13, 2021

cpanato commented Mar 21, 2021

cpanato commented Mar 21, 2021

saschagrunert left a comment

k8s-ci-robot commented Mar 22, 2021

move references from TOOL_BRANCH to be TOOL_REF #1906

move references from TOOL_BRANCH to be TOOL_REF #1906

Conversation

cpanato commented Feb 11, 2021

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

puerco left a comment

Choose a reason for hiding this comment

puerco Feb 11, 2021

Choose a reason for hiding this comment

puerco Feb 11, 2021

Choose a reason for hiding this comment

cpanato Feb 16, 2021

Choose a reason for hiding this comment

cpanato commented Feb 19, 2021

puerco left a comment

Choose a reason for hiding this comment

puerco Feb 23, 2021

Choose a reason for hiding this comment

saschagrunert left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Mar 13, 2021

cpanato commented Mar 21, 2021

cpanato commented Mar 21, 2021

saschagrunert left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Mar 22, 2021