-
Notifications
You must be signed in to change notification settings - Fork 57
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
feat: adds image_project_id parameter for pushing images to a different GCP project #170
feat: adds image_project_id parameter for pushing images to a different GCP project #170
Conversation
The tests were broken after the rebase from main. This commit fixes those tests.
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.
Hi @opdude,
Thanks for picking this one up! The code looks good to me, I just left a singe nit on an error message, but outside from that we're good I think.
Since this requires two projects to test the feature, I don't think we can easily add an acceptance test for this feature (I'll need to check what I can do on that front), have you confirmed it worked as you expect it to?
I'm pre-approving in advance since there's not a lot to change here, will circle back after you've addressed what I commented on.
Thanks again!
@@ -58,7 +58,7 @@ func (s *StepCreateImage) Run(ctx context.Context, state multistep.StateBag) mul | |||
} | |||
|
|||
if err != nil { | |||
err := fmt.Errorf("Error waiting for image: %s", err) | |||
err := fmt.Errorf("Error waiting for image: %s in", err) |
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 presume you wanted to add the project ID in this log? If this is unaccessible I suggest we keep the previous message.
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.
Ah this changed when debugging, intention was to remove this before commiting, have reverted it to the previous :)
Removed unnecessary change to the error message.
Thanks. Yeah, I've been running it successfully both with and without using the override to the destination GCP project. |
Hi @opdude, Thanks for the quick reroll! I'll run tests and if they succeed, we can consider this done, I'll merge the PR when that happens. Thanks again for picking this one up! |
Thanks for landing this @opdude! I'm glad to see these make it into main. |
Hi, Thank you for this PR ! Thanks ! |
Yes it stil only builds into a singular project but allows the building and destination project to be different. |
This PR resurrects and rebases this PR which creates an image_project_id configuration option (which defaults to project_id) and allows defining a different project that an image should be created in.
Closes #30