Port upgrade test to make#5252
Merged
jetstack-bot merged 3 commits intocert-manager:masterfrom Jul 1, 2022
Merged
Conversation
Contributor
|
Skipping CI for Draft Pull Request. |
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: SgtCoDFish The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
This was referenced Jun 30, 2022
bb4c776 to
e1a7b4f
Compare
SgtCoDFish
commented
Jun 30, 2022
Member
Author
SgtCoDFish
left a comment
There was a problem hiding this comment.
Self-review to help other reviewers!
Comment on lines
-419
to
-435
| # The targets (verify_deps, verify_chart, verify_upgrade, and cluster) are | ||
| # temorary and exist to keep the compatibility with the following Prow jobs: | ||
| # | ||
| # pull-cert-manager-chart | ||
| # pull-cert-manager-deps | ||
| # pull-cert-manager-upgrade | ||
| # | ||
| # Until we have removed these Bazel-based targets, we must disable the check | ||
| # of the system tools since the Bazel targets don't rely on those, and the image | ||
| # | ||
| # eu.gcr.io/jetstack-build-infra-images/bazelbuild | ||
| # | ||
| # doesn't have these tools. | ||
| BAZEL_TARGET := $(filter verify verify_deps verify_chart verify_upgrade cluster,$(MAKECMDGOALS)) | ||
| ifneq ($(BAZEL_TARGET),) | ||
| $(warning Not checking whether the system tools are present since Bazel already takes care of that in the target $(MAKECMDGOALS). .) | ||
| else |
Member
Author
There was a problem hiding this comment.
note: since none of the legacy targets actually call bazel any more, there's no need for this check
| containers: | ||
| #@overlay/match by=overlay.subset({"name": "cert-manager"}) | ||
| - image: #@ "quay.io/jetstack/cert-manager-controller:{}".format(data.values.app_version) | ||
| - image: #@ "docker.io/library/cert-manager-cainjector-amd64:{}".format(data.values.app_version) |
Member
Author
There was a problem hiding this comment.
note: this also updates the container name - @irbekrm agreed it probably should've been cainjector all along.
| containers: | ||
| #@overlay/match by=overlay.subset({"name": "cert-manager"}) | ||
| - image: #@ "quay.io/jetstack/cert-manager-controller:{}".format(data.values.app_version) | ||
| - image: #@ "docker.io/library/cert-manager-controller-amd64:{}".format(data.values.app_version) |
Member
Author
There was a problem hiding this comment.
note: docker.io/library/cert-manager-<ctr>-amd64 refers to the container name inside the kind cluster, which is what it's called when it's loaded by make.
e1a7b4f to
e69ea21
Compare
Member
Author
|
/test pull-cert-manager-chart |
e69ea21 to
048ae91
Compare
Member
Author
|
/test pull-cert-manager-chart |
30712a8 to
066fcbb
Compare
Member
Author
|
This was blocked behind cert-manager/testing#713 - now ready for review! |
Signed-off-by: Ashley Davis <ashley.davis@jetstack.io>
066fcbb to
e6a1e3e
Compare
This uses cmctl instead of kubectl_cert-manager, uses make instead of bazel and fixes an incorrect container name in test/fixtures/upgrade/overlay/cainjector-ops.yaml Signed-off-by: Ashley Davis <ashley.davis@jetstack.io>
we don't _need_ to remove these and we can keep them around for longer, but we don't need them to be in files we actually use and edit. putting the targets in a separate file feels cleaner! Signed-off-by: Ashley Davis <ashley.davis@jetstack.io>
e6a1e3e to
18e98ce
Compare
SgtCoDFish
commented
Jun 30, 2022
Comment on lines
-85
to
-122
|
|
||
| # The targets (verify_deps, verify_chart, verify_upgrade, and cluster) are | ||
| # temorary and exist to keep the compatibility with the following Prow jobs: | ||
| # | ||
| # pull-cert-manager-chart | ||
| # pull-cert-manager-deps | ||
| # pull-cert-manager-upgrade | ||
| # | ||
| # These targets should be removed as soon as the four above jobs and scripts are | ||
| # updated to use the "make" flow. | ||
| .PHONY: verify | ||
| verify: | ||
| $(warning "The 'verify' target is deprecated and will be removed soon. Please use instead 'ci-presubmit'") | ||
| bazel test //... | ||
|
|
||
| .PHONY: verify_deps | ||
| verify_deps: | ||
| @# this target can be removed once we've removed the pull-cert-manager-deps test from presubmits | ||
| @# for now, just make it a no-op so the tests don't fail | ||
| $(warning "The 'verify_deps' target is deprecated, does nothing, and will be removed soon. This target is not useful anymore with the new make flow.") | ||
| @true | ||
|
|
||
| # requires docker | ||
| .PHONY: verify_chart | ||
| verify_chart: | ||
| $(warning "The 'verify_chart' target is deprecated and will be removed soon. Please use instead 'verify-chart'.") | ||
| bazel build //deploy/charts/cert-manager | ||
| ./hack/verify-chart-version.sh bazel-bin/deploy/charts/cert-manager/cert-manager.tgz | ||
|
|
||
| .PHONY: verify_upgrade | ||
| verify_upgrade: | ||
| $(warning "The 'verify_upgrade' target is deprecated and will be removed soon. Please use instead 'make e2e-setup-kind && ./hack/verify-upgrade.sh'.") | ||
| ./hack/verify-upgrade.sh | ||
|
|
||
| .PHONY: cluster | ||
| cluster: | ||
| $(warning "The 'cluster' target is deprecated and will be removed soon. Please use instead 'make e2e-setup-kind'.") | ||
| ./devel/ci-cluster.sh |
Member
Author
There was a problem hiding this comment.
note: these are moved into legacy.mk, not deleted
Member
Author
|
/test pull-cert-manager-make-test |
Member
|
The logs in pull-cert-manager-upgrade look good. /lgtm |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pull Request Motivation
This PR implements our upgrade test using make instead of bazel.
This uses cmctl instead of kubectl_cert-manager, uses make instead of bazel and fixes an incorrect container name in
test/fixtures/upgrade/overlay/cainjector-ops.yaml.Also cleans up some old deprecated make targets from before the make migration, putting them in a separate file.
Kind
/kind cleanup
Release Note