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

Merge controller subcommand into connect inject subcommand #1697

Merged
merged 1 commit into from
Nov 11, 2022
Merged

Merge controller subcommand into connect inject subcommand #1697

merged 1 commit into from
Nov 11, 2022

Conversation

thisisnotashwin
Copy link
Contributor

@thisisnotashwin thisisnotashwin commented Nov 10, 2022 •
edited
Loading

Changes proposed in this PR:

  • Merge the controller subcommand into the connect inject subcommand.
  • This requires the CRDs to be deployed into the cluster when connectInject.enabled = true.
  • The ACL policy for connect inject needs to be updated to be a super set and include controller policies where it previously didnt.
  • Delete the controller sections from the values files, webhook config, the acceptance tests and the project at large.

How I've tested this PR:

  • Acceptance and unit tests

How I expect reviewers to test this PR:

  • 👀
  • Anything missing?

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

@thisisnotashwin thisisnotashwin force-pushed the ashwin/merge-controllers branch 6 times, most recently from f317a11 to 437eee1 Compare November 10, 2022 20:30
curtbushko
curtbushko reviewed Nov 11, 2022
View reviewed changes
Copy link
Contributor

@curtbushko curtbushko 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! There are a lot of changes but the tests make this feel a lot safer to do.

I only had a couple questions but nothing I would block you on. Ping me tomorrow if you need an approval.

charts/consul/values.yaml Show resolved Hide resolved
charts/consul/templates/connect-inject-clusterrole.yaml Show resolved Hide resolved
ishustava
ishustava approved these changes Nov 11, 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.

Awesome work!! Please remember to add a changelog entry with the breaking change

CONTRIBUTING.md
@@ -349,7 +349,7 @@ rebase the branch on main, fixing any conflicts along the way before the code ca
...
IngressGateway string = "ingressgateway"
```
1. Update `control-plane/subcommand/controller/command.go` and add your controller:
1. Update `control-plane/subcommand/inject-connect/command.go` and add your controller:
Copy link
Contributor

Choose a reason for hiding this comment

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

🙏

@thisisnotashwin thisisnotashwin force-pushed the ashwin/merge-controllers branch from 437eee1 to 2c63ccc Compare November 11, 2022 23:53
@thisisnotashwin thisisnotashwin merged commit 2ec38fd into main Nov 11, 2022
@thisisnotashwin thisisnotashwin deleted the ashwin/merge-controllers branch November 11, 2022 23:54
@Ranjandas Ranjandas mentioned this pull request Nov 30, 2022
Merged
2 tasks
@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

@ishustava ishustava ishustava approved these changes

@curtbushko curtbushko curtbushko 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.
3 participants
@thisisnotashwin @curtbushko @ishustava