-
Notifications
You must be signed in to change notification settings - Fork 560
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
🌱 docs: Provide AMI IDs for rebuilt Ubuntu amis and fix e2e conformance with CI Artifacts jobs #2018
🌱 docs: Provide AMI IDs for rebuilt Ubuntu amis and fix e2e conformance with CI Artifacts jobs #2018
Conversation
Due to kubernetes-sigs/image-builder#406, have had to rebuild Ubuntu AMIs. Am also bumping e2e versions so we have signal. Signed-off-by: Naadir Jeewa <jeewan@vmware.com>
/hold /test ? |
@randomvariable: The following commands are available to trigger jobs:
Use
In response to this:
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. |
/test pull-cluster-api-provider-aws-e2e-conformance-with-ci-artifacts |
1 similar comment
/test pull-cluster-api-provider-aws-e2e-conformance-with-ci-artifacts |
2232b03
to
3c326bf
Compare
Signed-off-by: Naadir Jeewa <jeewan@vmware.com>
3c326bf
to
7a2eb53
Compare
/test pull-cluster-api-provider-aws-e2e-conformance-with-ci-artifacts |
/unhold conformance tests are running successfully. The result of kubetest is independent of the cluster creation and deletion. |
/lgtm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question about hardcoding the values for USE_CI_ARTIFACTS, but otherwise lgtm
@@ -57,6 +57,7 @@ var _ = Describe("conformance tests", func() { | |||
Measure(specName, func(b Benchmarker) { | |||
|
|||
name := fmt.Sprintf("cluster-%s", util.RandomString(6)) | |||
setEnvVar("USE_CI_ARTIFACTS", "true", false) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the override on the env variable, wouldn't we want to use the value set from Prow?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's coming in as an explicit command line argument --use-ci-artifacts
, and setting env var for the purpose of envsubst rendering from the CAPI package.
Maybe should have wired up differently
@@ -105,6 +106,7 @@ var _ = Describe("conformance tests", func() { | |||
}, 1) | |||
|
|||
AfterEach(func() { | |||
setEnvVar("USE_CI_ARTIFACTS", "false", false) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar here, what is the need for overriding the env variable here rather than using the value provided by Prow.
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: detiber 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 |
Due to kubernetes-sigs/image-builder#406,
have had to rebuild Ubuntu AMIs.
Am also bumping e2e versions and fixing the conformance with ci artifacts job so we have signal.
Signed-off-by: Naadir Jeewa jeewan@vmware.com
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 kubernetes/kubernetes#95730