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

UPSTREAM: 53916: update .dockercfg content to config.json #16868

Conversation

juanvallejo
Copy link
Contributor

@juanvallejo juanvallejo commented Oct 13, 2017

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1476330

update the data format of .dockercfg to match the new docker config.json
format, which encapsulates all registry auth objects in an overall
"auths" object when an option --config-format is specified with the value
--config-format=config.json:

{
    "auths": {
        "reg.url": {
            "auth": "...=="
        }
    }
}

cc @openshift/cli-review @bparees @mfojtik

@openshift-ci-robot openshift-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Oct 13, 2017
@bparees
Copy link
Contributor

bparees commented Oct 13, 2017

  1. this isn't an upstream feature?
  2. any backwards compatibility concerns here?

@juanvallejo
Copy link
Contributor Author

juanvallejo commented Oct 13, 2017

@bparees

this isn't an upstream feature?

Would it be better to just update DockerConfigEntry upstream / make a new object (maybe DockerAuthsEntry?) that stores DockerConfigEntry in a field named Auths?

any backwards compatibility concerns here?

Not entirely sure how to approach this, but an idea could be:

  • Add a new flag (--config-format?) which would default to the legacy .dockercfg format
    or receive a value (config.json) that would output the new format instead

cc @fabianofranz @deads2k

@bparees
Copy link
Contributor

bparees commented Oct 13, 2017

Would it be better to just update DockerConfigEntry upstream / make a new object (maybe DockerAuthsEntry?) that stores DockerConfigEntry in a field named Auths?

I definitely don't think we want our implementation to diverge from the k8s one.

@juanvallejo juanvallejo force-pushed the jvallejo/update-dockercfg-data-to-dockerconfig-fmt branch from 37d9b28 to 0a48b6c Compare October 13, 2017 21:38
@juanvallejo
Copy link
Contributor Author

juanvallejo commented Oct 13, 2017

@bparees thanks, went ahead and opened upstream pr - there was already a type upstream to represent the new auths format: https://github.com/kubernetes/kubernetes/blob/master/pkg/credentialprovider/config.go#L35.

Will update both PRs to only use the new format when a flag is given

@bparees
Copy link
Contributor

bparees commented Oct 13, 2017

thanks @juanvallejo

@openshift-ci-robot openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Oct 13, 2017
@juanvallejo juanvallejo changed the title update .dockercfg content to config.json UPSTREAM: 53916: update .dockercfg content to config.json Oct 13, 2017
@juanvallejo juanvallejo force-pushed the jvallejo/update-dockercfg-data-to-dockerconfig-fmt branch 3 times, most recently from c8c6f36 to 9d080ae Compare October 13, 2017 23:34
@openshift-merge-robot openshift-merge-robot added the vendor-update Touching vendor dir or related files label Oct 14, 2017
@juanvallejo juanvallejo force-pushed the jvallejo/update-dockercfg-data-to-dockerconfig-fmt branch from 29086f5 to de24f37 Compare October 20, 2017 18:06
@openshift-ci-robot openshift-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 20, 2017
@juanvallejo
Copy link
Contributor Author

@bparees @fabianofranz upstream PR has merged. ptal

@juanvallejo juanvallejo force-pushed the jvallejo/update-dockercfg-data-to-dockerconfig-fmt branch from de24f37 to d4a3583 Compare October 20, 2017 18:09
update the data format of .dockercfg to match the new docker config.json
format, which encapsulates all registry auth objects in an overall
"auths" object:

{
    "auths": {
        "reg.url": {
            "auth": "...=="
        }
    }
}
@juanvallejo juanvallejo force-pushed the jvallejo/update-dockercfg-data-to-dockerconfig-fmt branch from d4a3583 to 5304de7 Compare October 20, 2017 19:44
@bparees
Copy link
Contributor

bparees commented Oct 20, 2017

lgtm but will let @fabianofranz sign off.

@fabianofranz
Copy link
Member

/lgtm
/approve

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 20, 2017
@juanvallejo
Copy link
Contributor Author

/assign derekwaynecarr

@deads2k
Copy link
Contributor

deads2k commented Oct 23, 2017

/approve

@openshift-merge-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, fabianofranz, juanvallejo

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@openshift-merge-robot openshift-merge-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 23, 2017
@juanvallejo
Copy link
Contributor Author

@fabianofranz this solves a cli blocker, wondering if we can add the "kind bug" label to this?

@juanvallejo
Copy link
Contributor Author

/kind bug

@openshift-ci-robot openshift-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label Oct 25, 2017
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot
Copy link
Contributor

Automatic merge from submit-queue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/bug Categorizes issue or PR as related to a bug. lgtm Indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. vendor-update Touching vendor dir or related files
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants