-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Consul service meta #6193
Consul service meta #6193
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.
Thanks for tackling this! I left one comment about the Hash func but its looking really good.
I was wondering about that. I figured if I changed a service meta and ran
the job, I’m expecting the change to be reflected in consul too. Is that
what this function would do?
…On Thu, Aug 22, 2019 at 10:01 PM Nick Ethier ***@***.***> wrote:
***@***.**** requested changes on this pull request.
Thanks for tackling this! I left one comment about the Hash func but its
looking really good.
------------------------------
In nomad/structs/services.go
<#6193 (comment)>:
> @@ -458,6 +461,10 @@ func (s *Service) Hash(allocID, taskName string, canary bool) string {
for _, tag := range s.CanaryTags {
io.WriteString(h, tag)
}
+ for k, v := range s.Meta {
I don't think we want meta as part of the hash. This is used for
generating the service ID so if you changed the meta it would cause the
service to be reregistered which doesn't seem like what we want.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#6193?email_source=notifications&email_token=AAAKSPPRG26XXHKT6O4NDPLQF5AIRA5CNFSM4IOYZE5KYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCCO2DMI#pullrequestreview-278766001>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAAKSPM4JBYMX7YU7T2IPZDQF5AIRANCNFSM4IOYZE5A>
.
|
@jeromebaude sorry for the confusion but you're actually right to leave it there, see my comment. |
@nickethier Ok, I think that should be it. Not sure why that job failed on travis, is that test flaky sometimes? Passed locally. https://travis-ci.org/hashicorp/nomad/jobs/575809975 The netlify website preview seems to point to the UI. I can't make sure my changes will be good there. They're pretty simple, so I'm expecting them to be! About the changelog and the release cycle. Is it possible to insert this in a a soon-to-be-released version? We're probably going to be running our own fork if not. |
Can somebody retry that Travis build please? Or let me know if this is not a normal failure. |
Yup looks like a flaky test to me. Great work @jeromegn and thanks for contributing! |
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
This fixes #4300, it adds a
meta
field to the service stanza in the job spec. It's then sent along when registering the service.I've added the meta data to the
Hash
function as well as theEquals
function for theService
struct. Not sure if that's desirable, I assumed it was.I believe I've followed most steps in the contributing document, except I'm not sure what to do about the changelog. Happy to fix that up though.