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

consul-k8s CLI - hcp self-managed clusters - fix the required environment variables that should just default to production settings #1593

Merged
merged 4 commits into from
Oct 6, 2022

Conversation

jmurret
Copy link
Member

@jmurret jmurret commented Oct 6, 2022

Changes proposed in this PR:
The following envvars can be passed to the consul-k8s CLI so they get used in the cluster creation and bootstrapping:

HCP_CLIENT_ID (required)

HCP_CLIENT_SECRET (required)

HCP_AUTH_URL

HCP_SCADA_ADDRESS

HCP_API_HOST

All of the ones that are not marked as required were exposed by the hcp-sdk so that we could test in other environments besides production. When they are not provided, hcp sdk will default them to prod settings, and they are truly optional.

There is currently a bug in consul-k8s CLI where when these are not provided to consul-k8s CLI, they still get set on the server pod as empty strings. The hcp sdk then recognizes that they are there and says they cannot be empty strings.

How I've tested this PR:

  • unit tests
  • building the CLI locally and using this to install an hcp self managed cluster without providing the optional envvars.

How I expect reviewers to test this PR:
👀

Checklist:

  • Tests added
  • CHANGELOG entry added

    HashiCorp engineers only, community PRs should not add a changelog entry.
    Entries should use present tense (e.g. Add support for...)

@jmurret jmurret requested review from a team, analogue, kschoche and t-eckert and removed request for a team October 6, 2022 05:59
@jmurret jmurret changed the base branch from main to jm/gnm-agentless October 6, 2022 06:00
cli/preset/preset.go Outdated Show resolved Hide resolved
cli/preset/preset_test.go Outdated Show resolved Hide resolved
Copy link
Contributor

@kschoche kschoche left a comment

Choose a reason for hiding this comment

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

Looks great! I left a couple non-blocking comments!

…ment variables that should just default to production settings
Base automatically changed from jm/gnm-agentless to main October 6, 2022 16:18
@jmurret jmurret requested a review from curtbushko October 6, 2022 16:34
Copy link
Contributor

@t-eckert t-eckert left a comment

Choose a reason for hiding this comment

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

@jmurret jmurret merged commit 1dfb2f3 into main Oct 6, 2022
@jmurret jmurret deleted the jm/hcp-envvars branch October 6, 2022 17:09
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.

3 participants