⚠️Remove hard code manifest version and hash #3707

jichenjc · 2020-09-29T08:44:59Z

What this PR does / why we need it:

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
⚠️Fixes #3505

(*clusterClient).CertManager: changed from func() CertManagerClient to func() (CertManagerClient, error)
Client.CertManager: changed from func() CertManagerClient to func() (CertManagerClient, error)

wfernandes

Hey @jichenjc! I'm guessing you are still working on this PR so I'll do a full review when you're done with it.

But just a couple of thoughts:

Feel free to add Fixes #3505 in the description of this PR so it's linked to the corresponding issue.
I wonder if the vars embeddedCertManagerManifestVersion and embeddedCertManagerManifestHash can be part of the certManagerClient rather than used as globals. That way, we can set these values in the client constructor. WDYT?

wfernandes · 2020-09-29T17:04:41Z

cmd/clusterctl/client/cluster/cert_manager.go

-	// run `go test` against this package. THe Test_VersionMarkerUpToDate will output
-	// the expected hash if it does not match the hash here.
-	embeddedCertManagerManifestHash = "af8c08a8eb65d102ba98889a89f4ad1d3db5d302edb5b8f8f3e69bb992faa211"
+	certmanagerVersionLable = "helm.sh/chart"


Be aware that currently the label is helm.sh/chart=cert-manager-v0.16.1. We've been using just the value v0.16.1 in our embeddedCertManagerManifestVersion.

exactly, I noticed that , thanks for the reminder

and I will check all above comments then reflect in my PR when it's read for review :)
thanks~

fabriziopandini

@jichenjc thanks for this PR!
first pass, but overall the approach looks good to me

cmd/clusterctl/client/cluster/cert_manager.go

cmd/clusterctl/client/cluster/cert_manager_test.go

vincepri · 2020-09-30T18:52:40Z

/milestone v0.3.11

vincepri · 2020-10-09T19:00:28Z

Changed the base branch to be release-0.3, we'll have to forward-port these changes to v0.4 later

vincepri · 2020-10-19T14:27:21Z

/retest

fabriziopandini

@jichenjc thanks for restarting this effort
Some nits, but overall I'm looking forward to get this merger soon

cmd/clusterctl/client/cluster/cert_manager.go

cmd/clusterctl/client/cluster/cert_manager_test.go

fabriziopandini

@jichenjc thanks for addressing all the comments! really appreciated!
lgtm pending addressing CI errors

fabriziopandini

/lgtm

wfernandes

@jichenjc One last thing. I think we'll be good to go after this. Thanks!

wfernandes · 2020-11-09T21:37:24Z

cmd/clusterctl/client/cluster/cert_manager_test.go

+	g.Expect(cm.embeddedCertManagerManifestVersion).ToNot(Equal(nil))
+	g.Expect(cm.embeddedCertManagerManifestHash).ToNot(Equal(nil))


Since these values are strings it makes more sense to check if they are empty instead of nil because they'll technically always be not nil since the default will be "".

Suggested change

g.Expect(cm.embeddedCertManagerManifestVersion).ToNot(Equal(nil))

g.Expect(cm.embeddedCertManagerManifestHash).ToNot(Equal(nil))

g.Expect(cm.embeddedCertManagerManifestVersion).ToNot(BeEmpty())

g.Expect(cm.embeddedCertManagerManifestHash).ToNot(BeEmpty())

thanks for the reminder ~ :)

will update

k8s-ci-robot · 2020-11-10T02:41:40Z

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

Test name	Commit	Details	Rerun command
pull-cluster-api-test	`6997768`	link	`/test pull-cluster-api-test`
pull-cluster-api-apidiff-release-0-3	`2cd042c`	link	`/test pull-cluster-api-apidiff-release-0-3`

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.

k8s-ci-robot · 2020-11-10T02:41:42Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign fabriziopandini after the PR has been reviewed.
You can assign the PR to them by writing /assign @fabriziopandini in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

cmd/clusterctl/OWNERS

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

fabriziopandini · 2020-11-10T11:49:59Z

/lgtm

vincepri · 2020-11-10T16:38:51Z

Should this be a ⚠️ ?

(*clusterClient).CertManager: changed from func() CertManagerClient to func() (CertManagerClient, error)
Client.CertManager: changed from func() CertManagerClient to func() (CertManagerClient, error)

vincepri · 2020-11-10T16:39:02Z

/assign @CecileRobertMichon
for approval

CecileRobertMichon · 2020-11-10T16:59:57Z

@vincepri why is this targeting v0.3? Is it fixing a bug or is it simply cleanup / enabling more flexibility? If not, maybe it belongs in 0.4, especially if it's breaking...

vincepri · 2020-11-10T17:16:52Z

I'm fine moving it to v0.4.0 - although the bug/PR came around v0.3.10 and didn't make it in

jichenjc · 2020-11-11T01:09:54Z

@vincepri @CecileRobertMichon so please help to guide and confirm that I need target to v0.4 (I think it's master? ) thanks

CecileRobertMichon · 2020-11-11T01:23:51Z

will let @fabriziopandini make the call based on the importance of the fix since he opened the original issue... My personal preference is to move all non-critical bugs to v0.4 at this point but if this is fixing something significant and the risk is low I'm fine with keeping it in 0.3.

fabriziopandini · 2020-11-11T09:42:22Z

Given that this is a code cleanup, not user-facing, I'm ok to move this to v0.4.0

jichenjc · 2020-11-11T09:53:35Z

@vincepri @fabriziopandini @wfernandes thanks for point out
#3908 is created (I don't know how to change pull target from v0.3 to master), so created a new PR , will close that one when this new one is merged

CecileRobertMichon · 2020-11-11T17:21:46Z

/hold

vincepri · 2020-11-11T17:32:03Z

Closing in favor of #3908

/close

k8s-ci-robot · 2020-11-11T17:32:12Z

@vincepri: Closed this PR.

In response to this:

Closing in favor of #3908

/close

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.

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Sep 29, 2020

k8s-ci-robot requested review from vincepri and wfernandes September 29, 2020 08:45

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Sep 29, 2020

jichenjc force-pushed the bug/3505 branch from 8f6eb49 to a5d6929 Compare September 29, 2020 09:27

wfernandes reviewed Sep 29, 2020

View reviewed changes

jichenjc force-pushed the bug/3505 branch 2 times, most recently from 01bdef0 to 98709af Compare September 30, 2020 08:33

k8s-ci-robot added 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 Sep 30, 2020

jichenjc force-pushed the bug/3505 branch 2 times, most recently from 1b0f0a1 to 6997768 Compare September 30, 2020 09:06

fabriziopandini reviewed Sep 30, 2020

View reviewed changes

k8s-ci-robot added this to the v0.3.11 milestone Sep 30, 2020

vincepri changed the base branch from master to release-0.3 October 9, 2020 19:00

jichenjc force-pushed the bug/3505 branch 8 times, most recently from 1ac5584 to a948f88 Compare October 22, 2020 08:48

fabriziopandini reviewed Oct 22, 2020

View reviewed changes

jichenjc force-pushed the bug/3505 branch from a948f88 to b560337 Compare October 22, 2020 10:42

jichenjc changed the title ~~WIP: Remove hard code manifest version and hash~~ Remove hard code manifest version and hash Oct 22, 2020

fabriziopandini reviewed Nov 9, 2020

View reviewed changes

jichenjc force-pushed the bug/3505 branch 2 times, most recently from 005f66d to 68dacb9 Compare November 9, 2020 10:28

fabriziopandini reviewed Nov 9, 2020

View reviewed changes

k8s-ci-robot assigned fabriziopandini Nov 9, 2020

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

wfernandes reviewed Nov 9, 2020

View reviewed changes

Remove hard code manifest version and hash

2cd042c

jichenjc force-pushed the bug/3505 branch from 68dacb9 to 2cd042c Compare November 10, 2020 02:40

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 10, 2020

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

k8s-ci-robot assigned CecileRobertMichon Nov 10, 2020

jichenjc changed the title ~~Remove hard code manifest version and hash~~ ⚠️Remove hard code manifest version and hash Nov 11, 2020

jichenjc mentioned this pull request Nov 11, 2020

⚠️Remove hard code manifest version and hash #3908

Closed

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 11, 2020

k8s-ci-robot closed this Nov 11, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

⚠️Remove hard code manifest version and hash #3707

⚠️Remove hard code manifest version and hash #3707

jichenjc commented Sep 29, 2020 •

edited

Loading

wfernandes left a comment

wfernandes Sep 29, 2020

jichenjc Sep 30, 2020 •

edited

Loading

fabriziopandini left a comment

vincepri commented Sep 30, 2020

vincepri commented Oct 9, 2020

vincepri commented Oct 19, 2020

fabriziopandini left a comment •

edited

Loading

fabriziopandini left a comment

fabriziopandini left a comment

wfernandes left a comment

wfernandes Nov 9, 2020

jichenjc Nov 10, 2020

k8s-ci-robot commented Nov 10, 2020

k8s-ci-robot commented Nov 10, 2020

fabriziopandini commented Nov 10, 2020

vincepri commented Nov 10, 2020

vincepri commented Nov 10, 2020

CecileRobertMichon commented Nov 10, 2020

vincepri commented Nov 10, 2020 •

edited

Loading

jichenjc commented Nov 11, 2020

CecileRobertMichon commented Nov 11, 2020

fabriziopandini commented Nov 11, 2020

jichenjc commented Nov 11, 2020

CecileRobertMichon commented Nov 11, 2020

vincepri commented Nov 11, 2020

k8s-ci-robot commented Nov 11, 2020

		g.Expect(cm.embeddedCertManagerManifestVersion).ToNot(Equal(nil))
		g.Expect(cm.embeddedCertManagerManifestHash).ToNot(Equal(nil))

⚠️Remove hard code manifest version and hash #3707

⚠️Remove hard code manifest version and hash #3707

Conversation

jichenjc commented Sep 29, 2020 • edited Loading

wfernandes left a comment

Choose a reason for hiding this comment

wfernandes Sep 29, 2020

Choose a reason for hiding this comment

jichenjc Sep 30, 2020 • edited Loading

Choose a reason for hiding this comment

fabriziopandini left a comment

Choose a reason for hiding this comment

vincepri commented Sep 30, 2020

vincepri commented Oct 9, 2020

vincepri commented Oct 19, 2020

fabriziopandini left a comment • edited Loading

Choose a reason for hiding this comment

fabriziopandini left a comment

Choose a reason for hiding this comment

fabriziopandini left a comment

Choose a reason for hiding this comment

wfernandes left a comment

Choose a reason for hiding this comment

wfernandes Nov 9, 2020

Choose a reason for hiding this comment

jichenjc Nov 10, 2020

Choose a reason for hiding this comment

k8s-ci-robot commented Nov 10, 2020

k8s-ci-robot commented Nov 10, 2020

fabriziopandini commented Nov 10, 2020

vincepri commented Nov 10, 2020

vincepri commented Nov 10, 2020

CecileRobertMichon commented Nov 10, 2020

vincepri commented Nov 10, 2020 • edited Loading

jichenjc commented Nov 11, 2020

CecileRobertMichon commented Nov 11, 2020

fabriziopandini commented Nov 11, 2020

jichenjc commented Nov 11, 2020

CecileRobertMichon commented Nov 11, 2020

vincepri commented Nov 11, 2020

k8s-ci-robot commented Nov 11, 2020

jichenjc commented Sep 29, 2020 •

edited

Loading

jichenjc Sep 30, 2020 •

edited

Loading

fabriziopandini left a comment •

edited

Loading

vincepri commented Nov 10, 2020 •

edited

Loading