Skip to content

1.8 test jobs#660

Merged
jetstack-bot merged 5 commits intocert-manager:masterfrom
SgtCoDFish:1.8-previous
May 11, 2022
Merged

1.8 test jobs#660
jetstack-bot merged 5 commits intocert-manager:masterfrom
SgtCoDFish:1.8-previous

Conversation

@SgtCoDFish
Copy link
Member

@SgtCoDFish SgtCoDFish commented Apr 14, 2022

This PR makes the changes necessary to update Prow config post-1.8 release.

This is a little different from the last time we did it i.e #605 because:

  • With the new make functionality being partially available for release-1.8, but not release-1.7 we needed to ensure that make tests run for 1.8, but not 1.7 and that for all tests that aren't yet available to be run with make, there is a bazel alternative being run
  • I've also added all the tests that run with feature flags disabled to periodics and optional presubmits against release-1.8 and release-1.7

@jetstack-bot jetstack-bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. area/cert-manager Indicates a PR related to cert-manager approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Apr 14, 2022
@irbekrm irbekrm mentioned this pull request May 10, 2022
Signed-off-by: Ashley Davis <ashley.davis@jetstack.io>
@jetstack-bot jetstack-bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels May 10, 2022
@jetstack-bot jetstack-bot added dco-signoff: no Indicates that at least one commit in this pull request is missing the DCO sign-off message. and removed dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. labels May 10, 2022
@jetstack-bot jetstack-bot added dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. and removed dco-signoff: no Indicates that at least one commit in this pull request is missing the DCO sign-off message. labels May 10, 2022
@irbekrm irbekrm changed the title WIP: 1.8 test jobs 1.8 test jobs May 10, 2022
@jetstack-bot jetstack-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 10, 2022
@irbekrm
Copy link
Contributor

irbekrm commented May 11, 2022

I will add 1.24 tests against release-1.8 (and possibly release-1.7) in a separate PR to reduce the scope of this.

Copy link
Member Author

@SgtCoDFish SgtCoDFish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've got a few suggestions - I can't request changes or LGTM because I'm the original author of the PR though 😁

Comment on lines 90 to 99
cert-manager/website:
# cert-manager/website uses master branch for 'current' version and the
# release-next branch for the 'next' version
release-next: v1.8
master: v1.7
release-next: v1.9
master: v1.8
# Older versions are archived into named release branches
release-1.7: v1.7
release-1.6: v1.6
release-1.5: v1.5
release-1.4: v1.4
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: these branches don't exist on the cert-manager website any more, I deleted them all because they were unused. I didn't think that they might be referenced here.

Should we remove the milestone_applier stuff for the website? It seems harmless to do that in this PR too.

- name: ndots
value: "1"

- name: ci-cert-manager-previous-e2e-v1-19
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: there's a difference here between how this test - ci-cert-manager-previous-e2e-v1-19 - is being run and how ci-cert-manager-e2e-feature-gates-disabled-v1-19-previous is being run below.

This one uses runner devel/ci-run-e2e.sh while the feature-gates-disabled one uses make.

Should all e2es should use make on release-1.8 and later? If we updated that now it's one less thing to do after we release 1.9.

Comment on lines +188 to +193
- name: pull-cert-manager-e2e-v1-19
context: pull-cert-manager-e2e-v1-19
always_run: false
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: later in this file there's pull-cert-manager-make-e2e-v1-23 for release-1.8. Would the ideal long-term goal be that all e2es from release-1.8 and later run with make, meaning we wouldn't add release-1.8 for tests which use devel/ci-run-e2e.sh?

Maybe that's one for a future PR though!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For this, my thinking was that we'd need to have two versions of each test, one for release-1.7 with Bazel, one with release-1.8 with make and since the presubmits will only be run a few times max to get some bug fixes merged, it shouldn't really matter whether it uses make or Bazel.

However, perhaps it would cause problems down the line that the periodics use make whilst the presubmits use Bazel.. I've had an issue with those being different for periodics and presubmits against master, where a PR for a flake for make periodic could not be tested by a presubmit because there was only a Bazel presubmit for that version of Kubernetes.

So I will add make version for 1.8 in this PR

Co-authored-by: Ashley Davis <ashley.davis@jetstack.io>
Signed-off-by: irbekrm <irbekrm@gmail.com>
@jetstack-bot jetstack-bot added dco-signoff: no Indicates that at least one commit in this pull request is missing the DCO sign-off message. and removed dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. labels May 11, 2022
irbekrm and others added 3 commits May 11, 2022 16:58
Co-authored-by: Ashley Davis <ashley.davis@jetstack.io>
Signed-off-by: irbekrm <irbekrm@gmail.com>
Signed-off-by: irbekrm <irbekrm@gmail.com>
As older branches were removed from website and milestones aren't really used there

Signed-off-by: irbekrm <irbekrm@gmail.com>
@jetstack-bot jetstack-bot added dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. and removed dco-signoff: no Indicates that at least one commit in this pull request is missing the DCO sign-off message. labels May 11, 2022
@irbekrm
Copy link
Contributor

irbekrm commented May 11, 2022

I've got a few suggestions - I can't request changes or LGTM because I'm the original author of the PR though

Thanks @SgtCoDFish I've addressed the comments, lmk if it makes sense and I might fish for another lgmt from someone- conscious that it might be taking too much people's time to ask a third person try to understand which test should run with make and which with Bazel 😅

Copy link
Member

@wallrj wallrj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this all looks ok.

/lgtm

@jetstack-bot jetstack-bot added the lgtm Indicates that a PR is ready to be merged. label May 11, 2022
@jetstack-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: SgtCoDFish, wallrj

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

The pull request process is described here

Details Needs approval from an approver in each of these files:

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

@jetstack-bot jetstack-bot merged commit 86fcd79 into cert-manager:master May 11, 2022
@jetstack-bot
Copy link
Contributor

@SgtCoDFish: Updated the following 2 configmaps:

  • plugins configmap in namespace default at cluster default using the following files:
    • key plugins.yaml using file config/plugins.yaml
  • job-config configmap in namespace default at cluster default using the following files:
    • key cert-manager-periodics.yaml using file config/jobs/cert-manager/cert-manager/cert-manager-periodics.yaml
    • key cert-manager-release-next-periodics.yaml using file config/jobs/cert-manager/cert-manager/release-next/cert-manager-release-next-periodics.yaml
    • key cert-manager-release-previous-periodics.yaml using file config/jobs/cert-manager/cert-manager/release-previous/cert-manager-release-previous-periodics.yaml
    • key cert-manager-release-previous-presubmits.yaml using file config/jobs/cert-manager/cert-manager/release-previous/cert-manager-release-previous-presubmits.yaml
Details

In response to this:

This PR makes the changes necessary to update Prow config post-1.8 release.

This is a little different from the last time we did it i.e #605 because:

  • With the new make functionality being partially available for release-1.8, but not release-1.7 we needed to ensure that make tests run for 1.8, but not 1.7 and that for all tests that aren't yet available to be run with make, there is a bazel alternative being run
  • I've also added all the tests that run with feature flags disabled to periodics and optional presubmits against release-1.8 and release-1.7

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. area/cert-manager Indicates a PR related to cert-manager dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. lgtm Indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants