-
Notifications
You must be signed in to change notification settings - Fork 316
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.
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
Support Prefixes #140 #89
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work @s-christoff!
I just have a couple stylistic testing suggestions inline.
catalog/from-k8s/resource_test.go
Outdated
require.NoError(err) | ||
|
||
// Wait a bit | ||
time.Sleep(500 * time.Millisecond) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most times you need to sleep during a test you could instead use Consul's retry.Run
like this:
https://github.com/hashicorp/consul-k8s/blob/master/catalog/from-consul/source_test.go#L39
It'll retry until tests pass every 25ms up to 7s. This way you can keep the sleeping time close to the minimum amount needed.
It also helps if for some reason you need to sleep more than 500ms because of issues in CI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉 This looks great! It will be good to have symmetry in the ability to add prefixes to services that are synced between Consul and Kubernetes.
There are a couple things it would be good to update before merging:
- It looks like there's a new field in syncer.go that isn't used anywhere that should be removed.
- I suggested renaming
serviceName
to be a bit more explicit. This isn't strictly required, but might be nice for folks reading the code in this file. - It would be good to put the prefix tests interspersed in the test file-- specifically having them near the other tests for the same service type.
- A couple of nitpicks :)
Looking forward to merging this!
catalog/from-k8s/resource_test.go
Outdated
@@ -1770,3 +1770,297 @@ func testService(name string) *apiv1.Service { | |||
}, | |||
} | |||
} | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The testService
function above this is a helper function rather than a test. Usually helper functions are at the bottom of the file, so I'd suggest moving the tests around a bit. Additionally, the standard in this file is to have all of the permutations of loadBalancer
, nodePort
, etc. tests are grouped together, so it would make more sense to insert these tests near their service type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great! You'll just need to run go fmt
on resource_test.go and update it so the CI checks pass, then it'll be ready to merge.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! Going to go ahead and merge.
Shoot, I still need you to update |
Add command line option to enable an additional prefix option. This creates symmetry between sync directions.
Everything is 💚 🤞 Thank you for all the help. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉
Add
K8sConsulPrefix
flag to be used to prepend K8s services in Consul with a prefix