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

feat(pkger): add application functionality for the notification rules #16305

Merged
merged 1 commit into from
Dec 20, 2019

Conversation

jsteenb2
Copy link
Contributor

@jsteenb2 jsteenb2 commented Dec 20, 2019

Screen Shot 2019-12-20 at 10 19 48 AM

@jsteenb2 jsteenb2 force-pushed the 4965p/pkger_parser_notif_rules branch from ed78778 to 256b3c9 Compare December 20, 2019 18:21
@jsteenb2 jsteenb2 requested a review from a team December 20, 2019 18:21
@jsteenb2 jsteenb2 force-pushed the 4965p/pkger_parser_notif_rules branch from 256b3c9 to 51ac6d1 Compare December 20, 2019 18:46
@jsteenb2 jsteenb2 force-pushed the 4965p/pkger_parser_notif_rules branch from 51ac6d1 to 4318cbd Compare December 20, 2019 18:52
@jsteenb2 jsteenb2 requested review from a team as code owners December 20, 2019 18:52
@jsteenb2 jsteenb2 removed request for a team December 20, 2019 18:54
@@ -1032,6 +1043,18 @@ func (s *Service) Apply(ctx context.Context, orgID, userID influxdb.ID, pkg *Pkg
}
}

// this is required after first primary run and before secondary run, b/c this has dependencies
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit, but feels like this comment could be simplified / made more easy to understand.

Something like
"This has to be run after the above primary resources, because it relies on notification endpoints already being applied."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

excellent point! thank you for improving that, struggled to figure out what to write, your version is miles ahead of where mine was lol

Copy link
Contributor

@imogenkinsman imogenkinsman left a comment

Choose a reason for hiding this comment

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

👍 this is great, very clean and readable

@jsteenb2 jsteenb2 merged commit 61dceaa into master Dec 20, 2019
@jsteenb2 jsteenb2 deleted the 4965p/pkger_parser_notif_rules branch December 20, 2019 20:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants