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.

Sign up for GitHub

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

Jump to bottom

Update Helm Values #1551

Merged
merged 11 commits into from
Oct 6, 2022
Merged

Update Helm Values #1551

merged 11 commits into from
Oct 6, 2022

Conversation

t-eckert
Copy link
Contributor

@t-eckert t-eckert commented Sep 28, 2022 •
edited
Loading

Changes proposed in this PR:

  • Set server.replicas=1
  • Set connectInject.enabled=true
  • Set controller.enabled=true
  • Updated BATS tests accordingly

How I've tested this PR:

  • 🦇

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...)

@t-eckert t-eckert marked this pull request as ready for review September 28, 2022 19:52
Base automatically changed from agentless-base to main September 28, 2022 21:30
@david-yu
Copy link
Contributor

Perhaps needs a rebase off of main?

@t-eckert t-eckert force-pushed the update-helm-values branch 2 times, most recently from 8a84dc0 to 646d73e Compare September 28, 2022 23:33
@t-eckert
Copy link
Contributor Author

Perhaps needs a rebase off of main?

Lol, yes.

@t-eckert t-eckert force-pushed the update-helm-values branch 3 times, most recently from f4f5c13 to ed515ea Compare September 29, 2022 15:17
@t-eckert t-eckert force-pushed the update-helm-values branch 3 times, most recently from 42994b7 to 8e94bfb Compare September 29, 2022 15:47
@david-yu david-yu mentioned this pull request Sep 29, 2022
Closed
2 tasks
@t-eckert t-eckert requested a review from ishustava September 29, 2022 16:26
ishustava
ishustava reviewed Oct 3, 2022
View reviewed changes
acceptance/tests/connect/connect_external_servers_test.go Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
charts/consul/test/unit/crd-ingressgateways.bats Show resolved Hide resolved
charts/consul/test/unit/crd-meshes.bats Outdated Show resolved Hide resolved
ishustava
ishustava approved these changes Oct 3, 2022
View reviewed changes
Copy link
Contributor

@ishustava ishustava left a comment

Choose a reason for hiding this comment

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

Looks good! I had a few suggestions, but nothing blocking.

@t-eckert t-eckert force-pushed the update-helm-values branch 2 times, most recently from 1b2b08b to cbca020 Compare October 4, 2022 20:19
@t-eckert t-eckert force-pushed the update-helm-values branch from cbca020 to 094c772 Compare October 5, 2022 17:48
@t-eckert t-eckert requested a review from jmurret October 5, 2022 17:48
@t-eckert
Copy link
Contributor Author

t-eckert commented Oct 5, 2022

Acceptance tests are now failing after a rebase due to this issue.

@t-eckert t-eckert removed the request for review from jmurret October 5, 2022 18:29
thisisnotashwin
thisisnotashwin approved these changes Oct 6, 2022
View reviewed changes
Copy link
Contributor

@thisisnotashwin thisisnotashwin left a comment

Choose a reason for hiding this comment

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

Great work Thomas! Have a question that is non blocking! sorry about the delay in reviewing this.

acceptance/tests/connect/connect_external_servers_test.go
Comment on lines +31 to +33

// This prevents controllers from being installed twice, causing a failure.
"controller.enabled": "false",
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you elaborate on this a little? How does the controller get installed twice? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Totally! This test runs two Consul on K8s installs. One with just servers and one with just the non-server components like connect-injector and sets the servers as being "external", pointing to the first deployment.

If the controller.enabled is true (as it is by default in this change) for both installs, then the test fails when the second install tries to install controllers to the Kubernetes cluster when they have already been installed.

@t-eckert t-eckert merged commit 21d21af into main Oct 6, 2022
@t-eckert t-eckert deleted the update-helm-values branch October 6, 2022 17:31
@david-yu david-yu mentioned this pull request Oct 7, 2022
Closed
@natitomattis natitomattis mentioned this pull request Jun 20, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thisisnotashwin thisisnotashwin thisisnotashwin approved these changes

@ishustava ishustava ishustava approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@t-eckert @david-yu @ishustava @thisisnotashwin