-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Use CI .deb builds in kubeadm e2e job. #2180
Merged
pipejakob
merged 1 commit into
kubernetes:master
from
pipejakob:k8s-anywhere-kubeadm-version
Mar 9, 2017
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains 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
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.
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.
this looks very magical, given here we are going to only support env vars, could this logic be in the kubeadm image?
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.
Hrm, I don't think it can be in the image runner. The call stack looks like:
The kubernetes repo gets pulled by
bootstrap.py
, so the logic needs to be executed after that step, and I need to adjust theE2E_OPTS
beforee2e-runner.sh
is invoked. I thought the job script was the perfect place for this, but if we only want to support static environment variables there, then does that mean I'll need to create a customscenario
to support this kind of logic (like kops has)?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.
You can get this in to make the test green first, and we can figure out where to move the logic to. It's easy to add a flag to the e2e scenario.
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.
Wait, what? bootstrap.py should be launching an image, the runner should not be calling bootstrap.py
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.
@fejta That may be the case when the job runs under Jenkins and uses
dockerized-e2e-runner.sh
, but this is a prow job, so it has its own custom Docker image. Prow launches the image defined inconfig.yaml
, and the rest of the chain is the above, with the job callinge2e-runner.sh
instead ofdockerized-e2e-runner.sh
to avoid Docker-in-Docker.@krzyzacy Sounds good to me. Did you have any other feedback about the diff, or was that a very informal 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.
Apologies! I clearly don't have the right context here. I'll get out of your way
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.
@fejta this is within prow, so it's more like local mode, but it's not using e2e scenario yet.
I'll let @fejta have the final stamp, see if he agrees with me.
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.
Seems he decided to throw this to me 👿
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.
@fejta No worries, thanks for trying to help out. And yeah, this is definitely to support local testing of the job, but shouldn't actually affect how it runs in prow.
@krzyzacy Thanks. :-)