Skip to content

Conversation

@mfojtik
Copy link
Contributor

@mfojtik mfojtik commented Mar 25, 2015

No description provided.

@mfojtik
Copy link
Contributor Author

mfojtik commented Mar 25, 2015

@bparees @soltysh @CowboysFan PTAL

(do not merge this till openshift/origin#1411 lands)

Copy link
Contributor

Choose a reason for hiding this comment

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

this is an encrypted password, not plaintext, right? might clarify that so someone doesn't try to create one from scratch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, encrypted

Copy link
Contributor

Choose a reason for hiding this comment

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

k8s provides the Secret resource which you can use to.....

Copy link
Contributor

Choose a reason for hiding this comment

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

no longer true w/ your other PR.

@mfojtik
Copy link
Contributor Author

mfojtik commented Mar 26, 2015

@bparees @soltysh @CowboysFan all comments addressed, thanks! Also openshift/origin#1411 is pushed so I think this is good to go.

Copy link

Choose a reason for hiding this comment

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

I'd propose writing: If it does not exist execute docker login to have it created.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@soltysh if it exists then you can use this command to add multiple entries to the file. I think the current text is ok.

Copy link
Contributor

Choose a reason for hiding this comment

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

"You can also add authentication entries to this file by running.... the file will be created if it does not exist"

Copy link

Choose a reason for hiding this comment

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

Big fat 👍 on that

@soltysh
Copy link

soltysh commented Mar 26, 2015

One more nit, you're using dockercfg and .dockercfg interchangeably is it intentional? IMHO it'd be better to have one name used all over the text.

@mfojtik
Copy link
Contributor Author

mfojtik commented Mar 26, 2015

@soltysh that is intentional, the filename is in fact .dockercfg but I hate to have key in Secret with name .dockercfg (the dot just looks weird and also I dunno if you can do it).

Copy link

Choose a reason for hiding this comment

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

Based on your last comment this should be .dockercfg then.

@mfojtik mfojtik force-pushed the build_config branch 3 times, most recently from 3788f21 to e9b575c Compare March 26, 2015 09:31
@soltysh
Copy link

soltysh commented Mar 26, 2015

LGTM

@bparees
Copy link
Contributor

bparees commented Mar 26, 2015

@mfojtik +1 on using "dockercfg" as the key.

@mfojtik
Copy link
Contributor Author

mfojtik commented Mar 26, 2015

@bparees fixed last comment :-)

@bparees
Copy link
Contributor

bparees commented Mar 26, 2015

lgtm. :)

@adellape
Copy link
Contributor

@mfojtik I've reviewed (FYI @CowboysFan) since I've got the related docs card (https://trello.com/c/XWNfqLAC).

Thanks, and merging. Will follow up per docs card.

adellape added a commit that referenced this pull request Mar 26, 2015
Add PushSecretName documentation
@adellape adellape merged commit 953f9e3 into openshift:master Mar 26, 2015
@mfojtik mfojtik deleted the build_config branch March 27, 2015 07:11
sbeskin-redhat pushed a commit to sbeskin-redhat/openshift-docs that referenced this pull request Oct 24, 2022
WINDUP-2269: Updated windupVersion description.
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