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

Sync LB endpoints #257

Merged
merged 11 commits into from
Jun 5, 2020
Merged

Sync LB endpoints #257

merged 11 commits into from
Jun 5, 2020

Conversation

ltagliamonte-dd
Copy link
Contributor

add option to sync LB Endpoints instead of LB external hostname
fixes #254

@lkysow @ishustava i know that you are busy but since the diff is small I really would appreciate a CR on this one before i commit prod to it.

@ltagliamonte-dd ltagliamonte-dd mentioned this pull request May 5, 2020
Closed
@adilyse adilyse added area/sync Related to catalog sync type/enhancement New feature or request labels May 14, 2020
@ltagliamonte-dd
Copy link
Contributor Author

@lkysow @ishustava any updates on this review?

ishustava
ishustava reviewed May 18, 2020
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.

Hey @ltagliamonte-dd, sorry for the delay.

Thanks for this PR!

I left some comments inline. Also, could you add tests for this new functionality?

catalog/to-consul/resource.go Outdated Show resolved Hide resolved
catalog/to-consul/resource.go Outdated Show resolved Hide resolved
subcommand/sync-catalog/command.go Outdated Show resolved Hide resolved
catalog/to-consul/resource.go Show resolved Hide resolved
ltagliamonte-dd and others added 3 commits May 18, 2020 13:42
@ltagliamonte-dd
Copy link
Contributor Author

@ishustava any updates on this review?

@ishustava
Copy link
Contributor

Hey @ltagliamonte-dd, thanks for making the changes! These updates look good to me. What about my comment about adding tests?

@ltagliamonte-dd
Copy link
Contributor Author

@ishustava test case added

ishustava
ishustava suggested changes Jun 4, 2020
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.

Thanks for adding the test! Everything looks mostly good to me, but I'd like to follow our existing test patterns (more information in my comments). Let me know what you think.

catalog/to-consul/resource_test.go Outdated Show resolved Hide resolved
catalog/to-consul/resource_test.go Outdated Show resolved Hide resolved
catalog/to-consul/resource_test.go Outdated Show resolved Hide resolved
ltagliamonte-dd and others added 3 commits June 4, 2020 17:58
@ltagliamonte-dd @ishustava
Update catalog/to-consul/resource_test.go
bd439c0 
Co-authored-by: Iryna Shustava <ishustava@users.noreply.github.com>
@ltagliamonte-dd @ishustava
Update catalog/to-consul/resource_test.go
a83d4d4 
Co-authored-by: Iryna Shustava <ishustava@users.noreply.github.com>
@ltagliamonte-dd
refactor
6c4a823
ishustava
ishustava approved these changes Jun 5, 2020
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.

🎉 thanks so much for making the changes so quickly!

@ishustava ishustava merged commit d6fc3b6 into hashicorp:master Jun 5, 2020
@ltagliamonte-dd
Copy link
Contributor Author

@ishustava do you have an ETA on when there will be a next official release that is going to contain this change?

@ishustava
Copy link
Contributor

@ltagliamonte-dd we're looking to release in the next couple of weeks, don't have a specific date yet because we're working on a couple more new features that will go with consul 1.8.

@ishustava
Copy link
Contributor

@ltagliamonte-dd We've released these changes in the 0.16.0 Release.

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

Assignees
No one assigned
Labels
area/sync Related to catalog sync type/enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

sync endpoints for serviceType=LoadBalancer
3 participants
@ltagliamonte-dd @ishustava @adilyse