-
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
client: hookup service wrapper for use in clients #12329
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.
LGTM; just the Parallel changes. You may want to rebase with main first.
// TestGroupServiceHook_GroupServices_Nomad asserts group service hooks with | ||
// group services does not error when using the Nomad provider. | ||
func TestGroupServiceHook_GroupServices_Nomad(t *testing.T) { | ||
t.Parallel() |
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.
t.Parallel() | |
ci.Parallel(t) |
These should get caught by Semgrep if you rebase with main
// Test_serviceHook_Nomad performs a normal operation test of the serviceHook | ||
// when using task services which utilise the Nomad provider. | ||
func Test_serviceHook_Nomad(t *testing.T) { | ||
t.Parallel() |
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.
t.Parallel() | |
ci.Parallle(t) |
Discussed internally; will rebase the feature branch shortly and address the changes needed. |
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 PR modifies the client task and alloc runners to utilise the new service registration wrapper. The service hooks have also been updated to use the wrapper along with some naming improvements which were previously Consul specific.
In order to handle a test case, the wrapper has been slightly modified. The function has a comment which details this change. Additional testing via table driven test should be added in the future.
closes https://github.com/hashicorp/team-nomad/issues/266