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

remove load-balancer-controller and ingress tests #225

Merged
merged 1 commit into from
Aug 5, 2021
Merged

remove load-balancer-controller and ingress tests #225

merged 1 commit into from
Aug 5, 2021

Conversation

ettiee
Copy link
Contributor

@ettiee ettiee commented Aug 3, 2021

Contributes #224

  • Remove aws-load-balancer-controller from cluster addons
  • Remove validateIngress test

@ettiee ettiee requested a review from a team as a code owner August 3, 2021 13:19
@ettiee ettiee requested review from mtpereira and aidy and removed request for a team August 3, 2021 13:19
@ettiee ettiee force-pushed the ee/remove-aws-lb-controller branch from 32d30bb to 49956cf Compare August 3, 2021 13:22
@cookpad cookpad deleted a comment from github-actions bot Aug 3, 2021
@ettiee ettiee requested a review from errm August 3, 2021 13:23
@ettiee ettiee mentioned this pull request Aug 3, 2021
Closed
@mtpereira
Copy link
Contributor

✔️ Change looks good!

🥐 Re-running flaky tests.

@aidy
Copy link
Contributor

aidy commented Aug 4, 2021

I think we should add some context to the commit messages here - I can see what they're doing, but it's unclear what the motivation is.

@aidy
Copy link
Contributor

aidy commented Aug 4, 2021

8fd9a68 should also be squashed into 49956cf for atomic-ness.

@aidy
Copy link
Contributor

aidy commented Aug 4, 2021 •
edited
Loading

Suggest that we squash these three commits together with a commit message something like:

Remove load-balancer-controller

We've decided to remove non-critical addons from this module.
Rather than try to provide a one-size-fits-all setup, we're changing focus to provide a
minimal setup, and expect that users will use another mechanism for additional configuration.

(remove load-balancer-controller and ingress tests is unclear English. Could read as load-balancer-controller tests and ingress tests or ingress tests and load-balancer-controller - but I think removing the tests here is a facet of removing the load-balancer-controller, rather than an explicit change in it's own right)

@ettiee ettiee mentioned this pull request Aug 4, 2021
Merged
@ettiee ettiee force-pushed the ee/remove-aws-lb-controller branch 2 times, most recently from 8fd9a68 to e54f46d Compare August 5, 2021 09:37
@ettiee
Remove load-balancer-controller
898d907 
We've decided to remove non-critical addons from this module #224.
Rather than try to provide a one-size-fits-all setup, we're changing focus to provide a
minimal setup, and expect that users will use another mechanism for additional configuration.
@ettiee ettiee force-pushed the ee/remove-aws-lb-controller branch from e54f46d to 898d907 Compare August 5, 2021 09:38
@ettiee
Copy link
Contributor Author

ettiee commented Aug 5, 2021

@aidy sure makes sense - could you check the latest squashed commit

mtpereira
mtpereira approved these changes Aug 5, 2021
View reviewed changes
Copy link
Contributor

@mtpereira mtpereira left a comment

Choose a reason for hiding this comment

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

🚀

@ettiee ettiee merged commit e7d92a6 into master Aug 5, 2021
@ettiee ettiee deleted the ee/remove-aws-lb-controller branch August 5, 2021 12:32
errm added a commit that referenced this pull request Nov 2, 2021
@errm
Remove cluster-name tagging from subnets
46f54f7 
* These tags were previously required by aws-load-balancer-controller/alb-ingress-controller
* We no longer ship a ingress controller with the module since #225
* aws-load-balancer-controller doesn't need those tags anyway since v2.1.2 - kubernetes-sigs/aws-load-balancer-controller#1773
@errm errm mentioned this pull request Nov 2, 2021
Merged
errm added a commit that referenced this pull request Nov 2, 2021
@errm
Remove cluster-name tagging from subnets
f8ef1ac 
* These tags were previously required by aws-load-balancer-controller/alb-ingress-controller
* We no longer ship a ingress controller with the module since #225
* aws-load-balancer-controller doesn't need those tags anyway since v2.1.2 - kubernetes-sigs/aws-load-balancer-controller#1773
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mtpereira mtpereira mtpereira approved these changes

@aidy aidy Awaiting requested review from aidy

@errm errm Awaiting requested review from errm

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

Successfully merging this pull request may close these issues.

None yet
3 participants
@ettiee @mtpereira @aidy