Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

adding a periodic job for testing on eks cluster #512

Merged
merged 14 commits into from
Aug 10, 2021
Merged

adding a periodic job for testing on eks cluster #512

merged 14 commits into from
Aug 10, 2021

Conversation

RinkiyaKeDad
Copy link
Contributor

Signed-off-by: RinkiyaKeDad arshsharma461@gmail.com

/assign @jakexks

Signed-off-by: RinkiyaKeDad <arshsharma461@gmail.com>
@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. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. area/cert-manager Indicates a PR related to cert-manager needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jun 11, 2021
@jetstack-bot
Copy link
Contributor

Hi @RinkiyaKeDad. Thanks for your PR.

I'm waiting for a jetstack member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@jetstack-bot jetstack-bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jun 11, 2021
Signed-off-by: RinkiyaKeDad <arshsharma461@gmail.com>
Signed-off-by: RinkiyaKeDad <arshsharma461@gmail.com>
@jetstack-bot jetstack-bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jun 25, 2021
Signed-off-by: RinkiyaKeDad <arshsharma461@gmail.com>
Signed-off-by: RinkiyaKeDad <arshsharma461@gmail.com>
@RinkiyaKeDad RinkiyaKeDad marked this pull request as draft July 3, 2021 05:17
Signed-off-by: RinkiyaKeDad <arshsharma461@gmail.com>
@RinkiyaKeDad RinkiyaKeDad changed the title [WIP] adding a periodic job for testing on eks cluster adding a periodic job for testing on eks cluster Jul 27, 2021
@RinkiyaKeDad RinkiyaKeDad marked this pull request as ready for review July 27, 2021 14:33
@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 Jul 27, 2021
@RinkiyaKeDad
Copy link
Contributor Author

/release-note-none

@jetstack-bot jetstack-bot added release-note-none Denotes a PR that doesn't merit a release note. 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 Jul 27, 2021
Signed-off-by: Arsh Sharma <arshsharma461@gmail.com>
Signed-off-by: Arsh Sharma <arshsharma461@gmail.com>
Signed-off-by: Arsh Sharma <arshsharma461@gmail.com>
Signed-off-by: Arsh Sharma <arshsharma461@gmail.com>
@jetstack-bot jetstack-bot added area/testing Indicates a PR related to testing needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 30, 2021
@jetstack-bot jetstack-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 30, 2021
Copy link
Contributor Author

@RinkiyaKeDad RinkiyaKeDad left a comment

Choose a reason for hiding this comment

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

Have a couple of doubts :)

# extra_refs:
# - org: jetstack
# repo: cert-manager
# base_ref: master
Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do these do? Are they needed?

Copy link
Contributor

Choose a reason for hiding this comment

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

It specifies what repository this Prow job clones when it runs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! Would you also know where it clones the repo? For example right now when we run the golang image we are in /go at the start. So if we specify this, would we be inside /go/cert-manager or would we still be in /go but have the cert-manager folder already cloned for us?

Copy link
Contributor

Choose a reason for hiding this comment

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

From a quick check it's cloned at /home/prow/go/src/github.com/jetstack/cert-manager and Prow sets that as workingDir for the test container.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

By workingDir do you mean that at the launch of the container we would be at /home/prow/go/src/github.com/jetstack/cert-manager? If yes, then if we add another repo would the first one be the workingDir?

Copy link
Contributor

@irbekrm irbekrm Jul 30, 2021

Choose a reason for hiding this comment

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

By workingDir do you mean that at the launch of the container we would be at /home/prow/go/src/github.com/jetstack/cert-manager?

Yep- it's the the workingDir in the container spec.

If yes, then if we add another repo would the first one be the workingDir

Looking at the code here looks like that would happen by default, but it seems like we can also set work_dir: true for a particular ref to configure which one should become workingDir.
I've not tried cloning more than one myself.

Comment on lines 373 to 380
# labels:
# preset-service-account: "true"
# preset-dind-enabled: "true"
# preset-bazel-remote-cache-enabled: "true"
# preset-bazel-scratch-dir: "true"
# preset-cloudflare-credentials: "true"
# preset-venafi-tpp-credentials: "true"
# preset-venafi-cloud-credentials: "true"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Which ones will we need?

# preset-venafi-cloud-credentials: "true"
spec:
containers:
- image: eu.gcr.io/jetstack-build-infra-images/bazelbuild:20210323-ad5071a-3.7.2
Copy link
Contributor Author

Choose a reason for hiding this comment

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

How do I specify the golang-aws image that we built here?

Copy link
Contributor

@irbekrm irbekrm Jul 30, 2021

Choose a reason for hiding this comment

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

You will first need to add the infra to build that image so it gets pushed to Jetstack Build Infra images GCR and then you can get the image sha from there. So probably the commits that build the image need to go in a PR before the PR that adds this Prow job.

- name: AWS_ACCESS_KEY_ID
value: "???"
- name: AWS_SECRET_ACCESS_KEY
value: "???"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can't have the access keys on GitHub so how will this work?

Copy link
Contributor

Choose a reason for hiding this comment

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

It'll probably need to be a Secret in the cluster where the Prow job runs. The Prow job can then be configured to consume it via a preset like we do it with i.e Venafi creds- preset defined here and consumed added to the job here. (The actual Secret would need to be created in the cluster separately).

Signed-off-by: Arsh Sharma <arshsharma461@gmail.com>
@jetstack-bot jetstack-bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 30, 2021
Signed-off-by: Arsh Sharma <arshsharma461@gmail.com>
@jetstack-bot jetstack-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 30, 2021
Signed-off-by: Arsh Sharma <arshsharma461@gmail.com>
Copy link
Contributor Author

@RinkiyaKeDad RinkiyaKeDad left a comment

Choose a reason for hiding this comment

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

/hold because of things mentioned in the review.

preset-aws-credentials: "true"
spec:
containers:
- image: <to-be-replaced>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This.

cd.. && \
cd.. && \
cd cert-manager && \
./devel/run-e2e.sh --ginkgo.focus "Public ACME Server HTTP01 Issuer" --acme-server-url=https://acme-staging-v02.api.letsencrypt.org/directory --ingress-controller-domain=aws.e2e-tests.cert-manager.io --testing-acme-email=<need-to-replace-this> --kubernetes-config=../test-infra/aws/kubeconfig_cert-manager-cluster;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

--testing-acme-email=<need-to-replace-this> this.

@jetstack-bot jetstack-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 4, 2021
Signed-off-by: Arsh Sharma <arshsharma461@gmail.com>
@RinkiyaKeDad
Copy link
Contributor Author

/unhold

@jetstack-bot jetstack-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 5, 2021
@jakexks
Copy link
Member

jakexks commented Aug 10, 2021

/ok-to-test

@jetstack-bot jetstack-bot added ok-to-test and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 10, 2021
@jakexks
Copy link
Member

jakexks commented Aug 10, 2021

/lgtm

@jetstack-bot jetstack-bot added the lgtm Indicates that a PR is ready to be merged. label Aug 10, 2021
@jetstack-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jakexks, RinkiyaKeDad

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:

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 added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 10, 2021
@jetstack-bot jetstack-bot merged commit 8e393cd into cert-manager:master Aug 10, 2021
@jetstack-bot
Copy link
Contributor

@RinkiyaKeDad: Updated the 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-periodics.yaml
  • key config.yaml using file config/jobs/cert-manager/config.yaml

In response to this:

Signed-off-by: RinkiyaKeDad arshsharma461@gmail.com

/assign @jakexks

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.

This pull request was closed.
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 area/testing Indicates a PR related to testing 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. ok-to-test release-note-none Denotes a PR that doesn't merit a release note. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants