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

Remove automatic --repo flag from bazel runner. #2416

Closed
wants to merge 1 commit into from

Conversation

pipejakob
Copy link
Contributor

If a periodic job uses the bazelbuild image and passes an argument of --repo=..., it ends up getting appended to the list of repos instead of overwriting it. Since not all jobs want this behavior, move the
--repo flag to config.yaml per job instead.

This also reverts the attempted (failed) workaround from #2393.

(See also previous effort: #2415)

If a periodic job uses the bazelbuild image and passes an argument of
"--repo=...", it ends up getting appended to the list of repos instead
of overwriting it. Since not all jobs want this behavior, move the
--repo flag to config.yaml per job instead.

This also reverts the attempted (failed) workaround from
kubernetes#2393.
@pipejakob pipejakob requested a review from spxtr April 5, 2017 23:06
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Apr 5, 2017
@krzyzacy
Copy link
Member

krzyzacy commented Apr 8, 2017

/assign @spxtr

@spxtr
Copy link
Contributor

spxtr commented Apr 11, 2017

How confident are you that this won't break anything?

@pipejakob
Copy link
Contributor Author

Not as confident as I was in #2415 which would have preserved backwards-compatibility :-).

How about I trim the PR down to just creating the new image and updating a few jobs to use it, and I can monitor and wait for them to pass, and slowly create additional PRs to roll out the update to the rest of the jobs? Is there a better way to ensure this is safe?

@krzyzacy
Copy link
Member

you can try to run the pod locally, https://github.com/kubernetes/test-infra/blob/master/experiment/pod.yaml might be a good reference

args:
- "--repo=k8s.io/$(REPO_NAME)"
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than saying $(REPO_NAME), why not just say kubernetes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The original intent was to make the job definitions even easier to copy/paste (say, between kubernetes and kubernetes-security) without having to tweak, but if you think it sacrifices readability, I'm pretty ambivalent.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is fine as-is.

@spxtr
Copy link
Contributor

spxtr commented Apr 17, 2017

BTW I'm working on a better way to test this kind of change. Do you mind waiting another day or two for that to come out and then we can use this PR as a test?

@pipejakob
Copy link
Contributor Author

@spxtr Completely fine. Sounds like a great improvement!

@pipejakob
Copy link
Contributor Author

I believe this has been superseded by ixdy's recent changes. Closing.

@pipejakob pipejakob closed this Jun 21, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants