Skip to content

Improve import updates with slice change plans and port deltas #116

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
Merged
merged 2 commits into from
Dec 3, 2021
Merged

Improve import updates with slice change plans and port deltas #116

merged 2 commits into from
Dec 3, 2021

Conversation

astaticvoid
Copy link
Contributor

@astaticvoid astaticvoid commented Dec 2, 2021 •
edited
Loading

Issue #, if available:
#101

Description of changes:
Multiple improvements to service import logic.

  • EndpointSlice change plan calculates necessary changes for endpoints.
  • Derived service ports are updated based on imported external changes.
  • Internal Port model is simplified and used broadly for comparisons ignoring order.
  • Constructor logic moved to util class
  • Some common test struct factory logic moved to controllers_common_test

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@astaticvoid astaticvoid requested review from runakash and bendu December 2, 2021 07:08
@codecov-commenter
Copy link

codecov-commenter commented Dec 2, 2021 •
edited
Loading

Codecov Report

Merging #116 (c1373c3) into main (14b1c36) will increase coverage by 4.68%.
The diff coverage is 84.89%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #116      +/-   ##
==========================================
+ Coverage   70.30%   74.98%   +4.68%     
==========================================
  Files          14       15       +1     
  Lines        1357     1443      +86     
==========================================
+ Hits          954     1082     +128     
+ Misses        330      287      -43     
- Partials       73       74       +1
Impacted Files Coverage Δ
pkg/controllers/serviceexport_controller.go 39.69% <ø> (ø)
pkg/model/types.go 66.03% <0.00%> (-1.27%) ⬇️
pkg/controllers/cloudmap_controller.go 46.35% <52.56%> (-10.11%) ⬇️
pkg/controllers/endpointslice_plan.go 96.03% <96.03%> (ø)
pkg/controllers/utils.go 97.94% <100.00%> (+40.56%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 14b1c36...c1373c3. Read the comment docs.

bendu
bendu previously approved these changes Dec 2, 2021
View reviewed changes
runakash
runakash previously approved these changes Dec 3, 2021
View reviewed changes
Copy link
Member

@runakash runakash left a comment

Choose a reason for hiding this comment

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

Overall looks good. Few minor comments.

pkg/controllers/common_test.go Outdated
@@ -0,0 +1,83 @@
package controllers
Copy link
Member

Choose a reason for hiding this comment

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

Should we rename this file into something else? There is package called common, this may create confusion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

renamed to controllers_common_test.go

pkg/controllers/utils.go Outdated
}

for _, portB := range b {
if !portB.Equals(aMap[portB.GetID()]) {
Copy link
Member

Choose a reason for hiding this comment

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

should we add check if portB.GetID() key is not found in the aMap

Copy link
Contributor Author

@astaticvoid astaticvoid Dec 3, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Good catch.
DeepEqual will handle it, but it's good to be explicit.

pkg/controllers/endpointslice_plan.go Outdated
ServiceImportName string

// Current EndpontSlices
Current []discovery.EndpointSlice
Copy link
Member

Choose a reason for hiding this comment

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

Could be optimal to use list of pointers here so we don't copy the complete list.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's fair.

@astaticvoid
PR fixes
3b3f2bc 
* common_test.go -> controllers_common_test.go
* use slice pointers for endpoint slice plan
* explicit key check in port matcher
@astaticvoid astaticvoid dismissed stale reviews from runakash and bendu via 3b3f2bc December 3, 2021 21:21
runakash
runakash approved these changes Dec 3, 2021
View reviewed changes
Copy link
Member

@runakash runakash left a comment

Choose a reason for hiding this comment

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

@astaticvoid astaticvoid merged commit 24f1996 into aws:main Dec 3, 2021
@astaticvoid astaticvoid deleted the endpoint-plan branch December 3, 2021 22:24
runakash pushed a commit to runakash/aws-cloud-map-mcs-controller-for-k8s that referenced this pull request Dec 3, 2021
@astaticvoid @runakash
Improve import updates with slice change plans and port deltas (aws#116)
74cc5b8
runakash pushed a commit to runakash/aws-cloud-map-mcs-controller-for-k8s that referenced this pull request Jun 20, 2025
@astaticvoid
Improve import updates with slice change plans and port deltas (aws#116)
5203df6
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@runakash runakash runakash approved these changes

+1 more reviewer

@bendu bendu bendu approved these changes

Reviewers whose approvals may not affect merge requirements
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
@astaticvoid @codecov-commenter @runakash @bendu