-
Notifications
You must be signed in to change notification settings - Fork 26
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
Adds initial support for login before bootstrapped build step #195
Adds initial support for login before bootstrapped build step #195
Conversation
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.
Hey @jonathanmccormack,
Thanks for the PR, as I mention in my comment, this should work but duplicates existing code, so I think we should probably reify that logic in a common area/package so the steps or other components that need it can use it instead of copy-pasting it when they want to use it.
That said, since the code is already duplicated, this can be done in a later PR, let me know what you prefer and we can roll with it.
Besides that, this looks good to me.
if !ok { | ||
state.Put("error", errors.New("missing config in state; this is a docker plugin bug, please report upstream")) | ||
return multistep.ActionHalt | ||
if config.EcrLogin { |
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'm assuming this code is quite similar, if not equivalent, to the code from step_pull.go
, is that right?
If so, I would suggest abstracting this logic (and cleanup function) to a common method, so that we don't duplicate this code.
I see it is already duplicated also for the post-processors when applicable, so I would understand if you prefer to continue with this for now, we can always reify this function in a later PR as well.
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'm assuming this code is quite similar, if not equivalent, to the code from
step_pull.go
, is that right?
Yes, it is effectively copied and pasted (with minor conforming cleanup to grab the config from state
and refactor some error messages).
If so, I would suggest abstracting this logic (and cleanup function) to a common method, so that we don't duplicate this code.
I see it is already duplicated also for the post-processors when applicable, so I would understand if you prefer to continue with this for now, we can always reify this function in a later PR as well.
I am all in favor of abstracting this logic to keep the code DRY, but was trying to keep my code changes to a minimum so as to make review and approval easier / faster, as the lack of support for login before a bootstrapped build is currently preventing us from using Packer with our configuration. I also didn't want to inadvertently break something, since I am new to this code!
If this otherwise looks OK and you are comfortable approving, I would suggest we keep the duplicated code for the time being. In the meantime I can begin working on a new PR to abstract the the logic, but am guessing that'll take me a bit longer.
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.
Oh yeah no pressure on this PR specifically, the current code looks good to me, just wanted to point-out that we could mutualise that logic, but as I mentioned and as you point out as well, this can be done later.
I can probably take care of this once your PRs have been integrated, that way we can proceed with releasing a new version of the plugin next week as well so those changes are usable then.
26f4316
to
9e29306
Compare
Hey @jonathanmccormack, FYI, I've just pushed an update to your PR, to address a lint issue on config.go (trailing whitespaces). Provided tests go green now, I'll merge this PR today. |
This PR adds support for performing
docker login
before commencing the build step when bootstrapping a packer build using a Dockerfile. More specifically, these changes add analogous logic to the build step from the pull step to attempt to authenticate with the information specified in thelogin_server
,login_username
, andlogin_password
config options when thelogin
value is true, or via ECR when theecr_login
value is true. This allows the user to build images from a private docker repository when performing a bootstrapped build from a Dockerfile.Closes #194