-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Bind GCE/AWS credentials and run AWS e2e inside docker #66
Conversation
@@ -587,27 +588,23 @@ | |||
trigger-job: '' | |||
timeout: 240 | |||
jenkins_node: 'master' |
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.
Does it still need to run on master?
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.
good catch. that's one of the main reasons to do this change in the first place. :)
I'm fine with it, but @fejta is on call. |
@@ -0,0 +1,19 @@ | |||
- wrapper: |
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.
Might appreciate a comment explaining what these are.
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.
I thought about that, but I wasn't sure that a comment would be any more descriptive than the env var name itself.
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.
Ok, seems reasonable.
Before merging I want one of the following to be true:
|
It passed in a staging Jenkins job for GCE. I don't think that's enough confidence though, since it hits everything. |
I understood what caused the breakage before, and it's been fixed, so I am reasonably confident this is a no-op for most builds. I'm not 100% sure though. Also, it's not possible to merge 30 PRs in one day. The unit test build takes around 1 hour right now. |
given that today is the last day for 1.3 features, it seems reasonable to hold off. |
alright, here goes... |
well, this broke AWS - we secretly depended on the
I'm not sure if we should add this to the |
Both seem reasonable to me. Is it worth using a new image for these? Similarly, is it worth using a different image for e2e testing and unit/integration testing? |
Automatic merge from submit-queue Install pip in kubekins test image and install AWS cli when needed Follow-on to kubernetes/test-infra#66. This fixes AWS hopefully. I haven't pushed the image yet. cc @fejta
Use target-repo in cherrypick queue service label selector
Benchmark marks metrics with too low avg values as matched & prints some more useful info
* Adds convenience method for pull request. * Reformat code * Missing property
Ignore generated extension files and IntelliJ IDEA project files
Update testimages to v20240314-e80ad011
Trying #19 again.
kubernetes/kubernetes/pull/25376 fixed the issues we encountered the first time.