Skip to content

Conversation

@ixdy
Copy link
Contributor

@ixdy ixdy commented Jun 23, 2017

Adds a --docker-registry flag to push-build.sh; if set, all images in _output/release-images will be pushed to the specified registry/project.

Depends on kubernetes/kubernetes#47939.
A part of addressing kubernetes/test-infra#1400.

cc @luxas @madhusudancs @roberthbailey

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 23, 2017
@david-mcmahon
Copy link
Contributor

If the intention here is to replace release::docker::release(), why not do that all here as well too?

@ixdy
Copy link
Contributor Author

ixdy commented Jun 23, 2017

This won't work for older releases (<k8s 1.8). I'm not sure I want to cherrypick my changes back.

At some point we could probably get rid of the existing release::docker::release().

@ixdy
Copy link
Contributor Author

ixdy commented Jun 23, 2017

put another way: I don't want to change anything about anago until 1.7.0 is out. after that I may consider changing things.

@david-mcmahon
Copy link
Contributor

Maybe check for /release-images/ somewhere here and dynamically use local images if they exist? Then this is backward compat and we can eventually remove release::docker::release() once we're beyond supporting <=1.7.

@david-mcmahon
Copy link
Contributor

That's fine. I'd prefer to leave things in a state where they Just Work under any condition/version, but this can certainly co-exist. Please put loud TODOs here with context though so we eventually clean up.

@ixdy
Copy link
Contributor Author

ixdy commented Jun 23, 2017

Good suggestions. I've made release::docker::release() call to release_from_tarfiles if the release-images directory exists, maintaining existing workflow otherwise.

logrun docker rmi "$registry/$legacy_docker_target" || true
logrun -r 5 -s docker tag "$registry/$docker_target" \
"$registry/$legacy_docker_target" 2>/dev/null
logecho
Copy link
Contributor Author

Choose a reason for hiding this comment

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

github does a terrible job displaying the diff, but I basically just indented here.

@ixdy
Copy link
Contributor Author

ixdy commented Jun 26, 2017

the PR in k/k has merged. PTAL?

@luxas
Copy link
Member

luxas commented Jun 26, 2017

I don't have enough experience with these scripts to say anything, but I guess it's ok for me if it works ;)

@roberthbailey
Copy link

/lgtm

@ixdy
Copy link
Contributor Author

ixdy commented Jun 27, 2017

gonna merge. this seems to work locally, but I'll watch CI/PR builds to make sure things are still working. fyi @krzyzacy @dchen1107

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. lgtm "Looks good to me", indicates that a PR is ready to be merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants