-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
De-nest docker registry auth and reformat related doc #445
Conversation
Thanks for the PR. I'm not sure the implementation here was completed. I think it should look something like below and thus not need dots or underscores, but this mapstructure needs to be pinned to the
|
@@ -392,6 +387,9 @@ func (d *DockerDriver) Start(ctx *ExecContext, task *structs.Task) (DriverHandle | |||
ServerAddress: driverConfig.ServerAddress, | |||
} | |||
|
|||
d.logger.Printf("[DEBUG] TASKCONFIG: %v", task.Config) | |||
d.logger.Printf("[DEBUG] DRIVERCONFIG: %v", driverConfig) | |||
d.logger.Printf("[DEBUG] AUTH: %v", authOptions) |
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.
This causes credential leakage.
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.
Yep, good catch (debugging cruft)
I agree, that's how I originally thought it worked. I'll make that change. |
Updated to take keys under |
Looks good! Thanks for following up. Would you be able to add a test for this? |
Yep, was just thinking that this morning. Will add. |
@carlosdp I'm going to merge this now because we want to avoid a BC break in config syntax in the 0.2 release. If you're still working on the tests please open a follow-up PR. Otherwise I will get around to this later. Thanks! |
De-nest docker registry auth and reformat related doc
* Allow buffered applyCh * comment
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
I'm not sure what happened here, but it looks like the Docker registry authentication parameters added in #390 aren't being decoded?
De-nesting the parameters seems to fix it. I'm guessing it's something to do with how
mapstructure
decodes the driver config struct.I also removed the periods (
.
) from the related keys as they are awkward to type in HCL files as they have to be wrapped in quotes, and re-formatted the related docs a bit so that it's more clear the keys are at the same "level" as the rest of the driver config parameters. I can revert these though if there's a good reason they should be the way they were before.