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

Allow multiple destinations #184

Merged
merged 2 commits into from
May 17, 2018
Merged

Conversation

mschrupp
Copy link
Contributor

Hello everyone,

I had the requirement to push an image to multiple container registries.
Since the image is already built it would make sense to push it to more than one registry (instead of building the image again with a different destination).

I recycled and existing type for multiple args and added a simple for-loop.
The missing os.Exit inside the loop made sense to me (why stop when only one of multiple pushes fail)
Tested this in my fork.

You can now specify multiple --destination flags.
If the tarpath is set, multiple tar-files (one for each destination) will be generated.

Please note that I didn't touch the README.md in this pull request.

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

@mschrupp
Copy link
Contributor Author

I signed it!

Copy link
Collaborator

@priyawadhwa priyawadhwa left a comment

Choose a reason for hiding this comment

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

Thanks for contributing, I left a couple comments!

logrus.Error(err)
os.Exit(1)

for _, destination := range destinations {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could we instead pass the destinations into DoPush, and have the for loop there?

for _, destination := range destinations {
if err := executor.DoPush(ref, image, destination, tarPath); err != nil {
logrus.Error(err)
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

We probably want to add the os.Exit(1) here!

@googlebot
Copy link

CLAs look good, thanks!

@mschrupp
Copy link
Contributor Author

Tested this, should be fine

@priyawadhwa priyawadhwa merged commit beb00f0 into GoogleContainerTools:master May 17, 2018
@priyawadhwa
Copy link
Collaborator

Great, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants