Skip to content

Conversation

@wking
Copy link
Member

@wking wking commented Sep 5, 2018

Instead of passing the pull secret around as a path, pass it around as a JSON string. This makes it easier to embed in Kubernetes, since we're punting the file-reading to callers at config-YAML-creation time.

And include more detail about the failure in JSONFile, vs. the old
boolean "this is invalid".
@openshift-ci-robot openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Sep 5, 2018
@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 5, 2018
@wking wking force-pushed the drop-pull-secret-path branch from 94b1b1f to 0b5a4d6 Compare September 5, 2018 18:29
@wking
Copy link
Member Author

wking commented Sep 5, 2018

/assign @abhinavdahiya

Instead of passing the pull secret around as a path, pass it around as
a JSON string.  This makes it easier to embed in Kubernetes, since
we're punting the file-reading to callers at config-YAML-creation
time.

Store it as a string (like the SSH pubkey) instead of parsing it out
into a more detailed structure, because we expect to consume it as an
opaque string (just pass it to the registry without peaking inside).

I've left some deprecated handling for folks who are still using
pullSecretPath in their YAML.  I'll file a follow-up pull request to
drop it once we get the CI template in openshift/release updated to
use pullSecret.
@wking wking force-pushed the drop-pull-secret-path branch from 0b5a4d6 to 8a84eb2 Compare September 5, 2018 19:10
@openshift-ci-robot openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 5, 2018
wking added a commit to wking/openshift-release that referenced this pull request Sep 5, 2018
Catching up with openshift/installer@8a84eb2e (*: Replace
PullSecretPath with PullSecret, 2018-09-05, openshift/installer#208).

The jq call ensures the secret JSON is on a single line, otherwise
we'd need to be a bit more careful about injecting it into the YAML as
a valid string literal.

I've replaced the inputs.yaml copy with a sed invocation to avoid
leaking the internal pull secret into the output artifacts.  I'm not
sure how sensitive it is though, maybe we don't mind leaking the
secret?
@abhinavdahiya
Copy link
Contributor

/lgtm
/hold

/cc @yifan-gu PTAL, @wking /hold cancel when @yifan-gu lgtms

@openshift-ci-robot openshift-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. labels Sep 5, 2018
Copy link
Contributor

@yifan-gu yifan-gu left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abhinavdahiya, wking, yifan-gu

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

The pull request process is described here

Details Needs approval from an approver in each of these files:
  • OWNERS [abhinavdahiya,wking,yifan-gu]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@wking
Copy link
Member Author

wking commented Sep 5, 2018

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 5, 2018
@openshift-merge-robot openshift-merge-robot merged commit 1bdd57e into openshift:master Sep 6, 2018
@wking wking deleted the drop-pull-secret-path branch September 6, 2018 02:20
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. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants