Skip to content

Conversation

@bparees
Copy link
Contributor

@bparees bparees commented Jul 29, 2015

No description provided.

@bparees
Copy link
Contributor Author

bparees commented Jul 29, 2015

@deads2k @liggitt ptal.

Copy link

Choose a reason for hiding this comment

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

lowercase secrets?

@liggitt
Copy link

liggitt commented Jul 29, 2015

while you're in here, the whole base-64 discussion and secret JSON-building can be replaced with

oc secrets new scmsecret ssh-privatekey=$HOME/.ssh/id_rsa

@liggitt
Copy link

liggitt commented Jul 29, 2015

nits/improvements, LGTM

@bparees
Copy link
Contributor Author

bparees commented Jul 29, 2015

@liggitt updated.
@soltysh i was a bit confused by why there were two "oc create -f secret.json" steps in the doc previously, one of which referred to a Docker secret. Can you please confirm that removing all that was the right thing to do here? i think one of them was always redundant, and now neither are needed thanks to @liggitt 's suggested command.

@liggitt
Copy link

liggitt commented Jul 29, 2015

adding the docker secret would be needed to push images to or pull images from a private repo other than the built-in one

@bparees
Copy link
Contributor Author

bparees commented Jul 29, 2015

@liggitt yeah but that's covered in its own section already:
https://docs.openshift.org/latest/dev_guide/builds.html#using-docker-credentials-for-pushing-and-pulling-images

(a section which i should note uses the correct instructions :) )

@liggitt
Copy link

liggitt commented Jul 29, 2015

yeah, ok... I didn't notice the second docker secret bit... that does seem superfluous.

@bfallonf
Copy link

@bparees @liggitt I came across the second secret bit too when I first went through this doc. I questioned @soltysh and he told me it was necessary. You can see the conversation here: #443 (my question on line 313). Maybe double-check with him that it's all good to get rid of this from the docs?

@bparees
Copy link
Contributor Author

bparees commented Jul 30, 2015

@bfallonf thanks, that is why I tagged him on this initially :) but i think in the PR you're pointing to it really was the two distinct sections (one for docker, one for source), i think that was different from what i removed.

@bparees
Copy link
Contributor Author

bparees commented Jul 30, 2015

@mfojtik can you pinch hit for @soltysh on reviewing this and in particular the question about why we were talking about docker secrets in the scm section?

@soltysh
Copy link

soltysh commented Jul 30, 2015

@bparees yeah, it's safe to remove it. I have no clue how it get there in he first place. Did you tested that the command @liggitt suggested works properly. I remember that SA works a bit differently from what we expect from SCM secrets,

@bparees
Copy link
Contributor Author

bparees commented Jul 30, 2015

I did not, but looking at what the instructions said to do i'm not seeing how it would be different...

what's "SA"?

@bparees
Copy link
Contributor Author

bparees commented Jul 30, 2015

@soltysh I just created a secret using the new way and compared its value to what the instructions would have had you do and it looks to be the same thing to me.

@adellape I think we're good to merge this.

@soltysh
Copy link

soltysh commented Jul 30, 2015

LGTM if so

@adellape
Copy link
Contributor

what's "SA"?

@bparees Service accounts?

@adellape
Copy link
Contributor

LGTM

adellape added a commit that referenced this pull request Jul 30, 2015
missing step for add secret to build svc account
@adellape adellape merged commit fec1174 into openshift:master Jul 30, 2015
@bparees bparees deleted the builder branch August 3, 2015 21:42
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.

5 participants