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

feat: Securitygroup Drift #3949

Merged
merged 19 commits into from
Jun 22, 2023
Merged

feat: Securitygroup Drift #3949

merged 19 commits into from
Jun 22, 2023

Conversation

engedaam
Copy link
Contributor

@engedaam engedaam commented May 26, 2023 •
edited

Description

  • Security group drift will check drift between AWSNodeTemplate.Spec.SecurityGroupSelector and machines with resolved values returned from EC2

How was this change tested?

  • Unite Tests
  • Integration Tests
  • Manually Tested

Does this change impact docs?

  • Yes, PR includes docs updates
  • Yes, issue opened: #
  • No

Release Note

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@netlify
Copy link

netlify bot commented May 26, 2023 •
edited

✅ Deploy Preview for karpenter-docs-prod canceled.

Name Link
🔨 Latest commit 0b2e1c3
🔍 Latest deploy log https://app.netlify.com/sites/karpenter-docs-prod/deploys/6494847c952f980008396356

@engedaam engedaam force-pushed the secuirtygroup_drift branch 5 times, most recently from 2e69e65 to 3f49bff Compare May 26, 2023 22:40
@engedaam engedaam marked this pull request as ready for review May 26, 2023 22:43
@engedaam engedaam requested a review from a team as a code owner May 26, 2023 22:43
@engedaam engedaam requested a review from tzneal May 26, 2023 22:43
@engedaam engedaam force-pushed the secuirtygroup_drift branch 4 times, most recently from d352d3b to 457102b Compare May 30, 2023 18:29
@engedaam engedaam changed the title feat: Security Group Drift feat: Securitygroup Drift May 30, 2023
@engedaam engedaam force-pushed the secuirtygroup_drift branch 3 times, most recently from f0c68f7 to 71a19ac Compare May 31, 2023 23:19
@engedaam
Copy link
Contributor Author

Related to #1738

njtran
njtran reviewed Jun 1, 2023
View reviewed changes
Copy link
Contributor

@njtran njtran left a comment

Choose a reason for hiding this comment

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

Nice work. I think the tests need to be adjusted too

pkg/cloudprovider/drift.go Outdated Show resolved Hide resolved
pkg/providers/instance/types.go Outdated Show resolved Hide resolved
pkg/cloudprovider/suite_test.go Outdated Show resolved Hide resolved
test/suites/drift/suite_test.go Outdated Show resolved Hide resolved
website/content/en/preview/concepts/deprovisioning.md Outdated Show resolved Hide resolved
@engedaam engedaam force-pushed the secuirtygroup_drift branch 2 times, most recently from 8cd5344 to a88d816 Compare June 1, 2023 21:19
jonathan-innis
jonathan-innis reviewed Jun 2, 2023
View reviewed changes
pkg/cloudprovider/drift.go Outdated Show resolved Hide resolved
pkg/cloudprovider/drift.go Outdated Show resolved Hide resolved
pkg/cloudprovider/drift.go Outdated Show resolved Hide resolved
pkg/cloudprovider/suite_test.go Outdated Show resolved Hide resolved
pkg/cloudprovider/suite_test.go Outdated Show resolved Hide resolved
test/suites/drift/suite_test.go Outdated Show resolved Hide resolved
test/suites/drift/suite_test.go Show resolved Hide resolved
test/suites/drift/suite_test.go Outdated Show resolved Hide resolved
website/content/en/preview/concepts/deprovisioning.md Outdated Show resolved Hide resolved
test/pkg/environment/aws/expectations.go Outdated Show resolved Hide resolved
@engedaam engedaam force-pushed the secuirtygroup_drift branch 3 times, most recently from 1efe0a4 to 192fd76 Compare June 5, 2023 23:30
njtran
njtran reviewed Jun 5, 2023
View reviewed changes
pkg/cloudprovider/drift.go Outdated Show resolved Hide resolved
pkg/cloudprovider/drift.go Outdated Show resolved Hide resolved
pkg/cloudprovider/drift.go Outdated Show resolved Hide resolved
website/content/en/preview/concepts/deprovisioning.md Outdated Show resolved Hide resolved
website/content/en/preview/concepts/deprovisioning.md Outdated Show resolved Hide resolved
pkg/cloudprovider/suite_test.go Outdated Show resolved Hide resolved
pkg/cloudprovider/suite_test.go Outdated Show resolved Hide resolved
pkg/cloudprovider/suite_test.go Outdated Show resolved Hide resolved
test/suites/drift/suite_test.go Outdated Show resolved Hide resolved
test/suites/drift/suite_test.go Outdated Show resolved Hide resolved
@engedaam engedaam force-pushed the secuirtygroup_drift branch 3 times, most recently from df26750 to dd52f8a Compare June 8, 2023 17:06
jonathan-innis
jonathan-innis reviewed Jun 15, 2023
View reviewed changes
test/suites/drift/suite_test.go Outdated Show resolved Hide resolved
pkg/cloudprovider/suite_test.go Outdated Show resolved Hide resolved
pkg/cloudprovider/suite_test.go Outdated Show resolved Hide resolved
pkg/cloudprovider/suite_test.go Outdated Show resolved Hide resolved
pkg/cloudprovider/suite_test.go Outdated Show resolved Hide resolved
pkg/cloudprovider/suite_test.go Outdated Show resolved Hide resolved
njtran
njtran reviewed Jun 15, 2023
View reviewed changes
pkg/cloudprovider/drift.go Show resolved Hide resolved
pkg/cloudprovider/drift.go Outdated Show resolved Hide resolved
pkg/cloudprovider/suite_test.go Outdated Show resolved Hide resolved
pkg/cloudprovider/suite_test.go Outdated Show resolved Hide resolved
test/suites/drift/suite_test.go Show resolved Hide resolved
njtran
njtran reviewed Jun 20, 2023
View reviewed changes
pkg/cloudprovider/suite_test.go Outdated Show resolved Hide resolved
pkg/cloudprovider/suite_test.go Outdated Show resolved Hide resolved
test/suites/drift/suite_test.go Outdated Show resolved Hide resolved
jonathan-innis
jonathan-innis reviewed Jun 21, 2023
View reviewed changes
pkg/cloudprovider/suite_test.go Outdated Show resolved Hide resolved
pkg/cloudprovider/suite_test.go Outdated Show resolved Hide resolved
pkg/cloudprovider/suite_test.go Outdated Show resolved Hide resolved
pkg/cloudprovider/drift.go Show resolved Hide resolved
test/suites/drift/suite_test.go Outdated Show resolved Hide resolved
test/suites/drift/suite_test.go Outdated Show resolved Hide resolved
@engedaam engedaam force-pushed the secuirtygroup_drift branch 2 times, most recently from 7e8d733 to ad8be37 Compare June 21, 2023 17:42
engedaam
engedaam commented Jun 22, 2023
View reviewed changes
Copy link
Contributor Author

@engedaam engedaam left a comment

Choose a reason for hiding this comment

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

/karpenter snapshot

@github-actions
Copy link
Contributor

Snapshot successfully published to oci://public.ecr.aws/karpenter/karpenter:v0-0b99e73c9b4dc1cba5c344a1cb060e08cdc5f7da. Find the image tag and installation instructions at https://gallery.ecr.aws/karpenter/karpenter/

jonathan-innis
jonathan-innis approved these changes Jun 22, 2023
View reviewed changes
Copy link
Contributor

@jonathan-innis jonathan-innis left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@njtran
Copy link
Contributor

njtran commented Jun 22, 2023

Great work on responding to feedback and getting this in!

@engedaam engedaam removed the request for review from tzneal June 22, 2023 20:31
@engedaam engedaam merged commit b2a1c23 into aws:main Jun 22, 2023
14 checks passed
@engedaam engedaam deleted the secuirtygroup_drift branch June 22, 2023 20:31
@engedaam engedaam mentioned this pull request Jun 22, 2023
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@njtran njtran njtran approved these changes

@jonathan-innis jonathan-innis jonathan-innis 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.

None yet
3 participants
@engedaam @njtran @jonathan-innis