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.

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 template manager #1783

Merged
merged 6 commits into from
Oct 6, 2016
Merged

Consul template manager #1783

merged 6 commits into from
Oct 6, 2016

Conversation

dadgar
Copy link
Contributor

@dadgar dadgar commented Oct 3, 2016

No description provided.

@dadgar dadgar force-pushed the f-consul-template branch 2 times, most recently from dafe298 to e8712e7 Compare October 6, 2016 17:56
Copy link
Member

@schmichael schmichael left a comment

Choose a reason for hiding this comment

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

LGTM! Nothing critical in comments.

taskEnv *env.TaskEnvironment) (*TaskTemplateManager, error) {

// Check pre-conditions
if hook == nil {
Copy link
Member

Choose a reason for hiding this comment

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

This will miss the case of someone creating a var like var hook *MyTaskHook, never initializing it, and passing it in. It will be a non-nil interface with a nil value.

I don't think it's worth trying to guard against that case, but I thought it was worth pointing out these kinds of guards are imperfect.

Copy link
Member

Choose a reason for hiding this comment

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

Hm, either NewTaskTemplateManager is only called from tests or its non-test callers have been trimmed from the PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah this just the library PR, the use of it is in the next

Wait: &watch.Wait{},
}

ctmpls[*ct] = tmpl
Copy link
Member

@schmichael schmichael Oct 6, 2016

Choose a reason for hiding this comment

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

Skip the & above to skip the dereferencing here

@dadgar dadgar force-pushed the f-consul-template branch from e8712e7 to 45531b1 Compare October 6, 2016 18:32
@dadgar dadgar force-pushed the f-consul-template branch from 45531b1 to 1c24f59 Compare October 6, 2016 19:36
@dadgar dadgar merged commit c683aca into master Oct 6, 2016
@dadgar dadgar deleted the f-consul-template branch October 6, 2016 22:05
@github-actions
Copy link

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.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants