Skip to content

bootstrap: set --kube-ca default to empty#459

Merged
openshift-merge-robot merged 1 commit intoopenshift:masterfrom
mrogers950:kube_ca_default
Feb 19, 2019
Merged

bootstrap: set --kube-ca default to empty#459
openshift-merge-robot merged 1 commit intoopenshift:masterfrom
mrogers950:kube_ca_default

Conversation

@mrogers950
Copy link
Copy Markdown

This PR is a follow-up regarding #420 (comment)
The installer now uses the flag with openshift/installer#1266.
/cc @cgwalters

@openshift-ci-robot openshift-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Feb 19, 2019
Copy link
Copy Markdown
Member

@ashcrow ashcrow left a comment

Choose a reason for hiding this comment

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

Change looks fine, but why default to empty rather than a default? As an example, the next line has a default in /assets/ as well.

@mrogers950
Copy link
Copy Markdown
Author

Change looks fine, but why default to empty rather than a default? As an example, the next line has a default in /assets/ as well.

The code makes including the kube-ca optional if it's empty, so I was keeping it in line with that. I was not sure if this command ran under any other context than the installer in order to have a given default be useful.

@mrogers950
Copy link
Copy Markdown
Author

/retest

@ashcrow
Copy link
Copy Markdown
Member

ashcrow commented Feb 19, 2019

The code makes including the kube-ca optional if it's empty, so I was keeping it in line with that. I was not sure if this command ran under any other context than the installer in order to have a given default be useful.

That's fair. 👍

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 19, 2019
@kikisdeliveryservice
Copy link
Copy Markdown
Contributor

LGTM but will defer to @cgwalters to approve

/assign @cgwalters

@kikisdeliveryservice
Copy link
Copy Markdown
Contributor

400 response & HAProxy flakes:

/test e2e-aws

@cgwalters
Copy link
Copy Markdown
Member

/lgtm

But FWIW in #403 we changed things to error out if the option isn't provided. There aren't any use cases for this CLI other than the installer.

But eh, not worth respinning now.

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 19, 2019
@openshift-ci-robot
Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ashcrow, cgwalters, mrogers950

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:

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

@openshift-bot
Copy link
Copy Markdown
Contributor

/retest

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

@ashcrow
Copy link
Copy Markdown
Member

ashcrow commented Feb 19, 2019

Flake

    --- FAIL: TestContainerRuntimeConfigCreate/unrecognized (0.00s)

/retest

@openshift-merge-robot openshift-merge-robot merged commit f8ae145 into openshift:master Feb 19, 2019
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/XS Denotes a PR that changes 0-9 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants