-
Notifications
You must be signed in to change notification settings - Fork 148
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
[GH-593] Add feature flag for new pull requests #612
Conversation
- Add new_pulls feature flag - Add check for the flag in postPullRequestEvent - Add check for conflicting flags and refactor the conflicting checks to a helper function
Codecov ReportBase: 15.63% // Head: 15.91% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #612 +/- ##
==========================================
+ Coverage 15.63% 15.91% +0.27%
==========================================
Files 15 15
Lines 5243 5259 +16
==========================================
+ Hits 820 837 +17
+ Misses 4380 4379 -1
Partials 43 43
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
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 one 👍
One request on the naming
server/plugin/command.go
Outdated
@@ -17,6 +17,7 @@ const ( | |||
featureIssues = "issues" | |||
featurePulls = "pulls" | |||
featurePullsMerged = "pulls_merged" | |||
featurePullsCreated = "new_pulls" |
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.
Let's follow the existing pattern and use pulls_new
ok, conflictingFs := checkFeatureConflict(fs) | ||
if !ok { | ||
if len(conflictingFs) == 2 { | ||
return fmt.Sprintf("Feature list cannot contain both %s and %s", conflictingFs[0], conflictingFs[1]) | ||
} | ||
return fmt.Sprintf("Conflicting feature(s) provided: %s", strings.Join(conflictingFs, ",")) | ||
} |
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.
👍
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 this great feature @C-Deck! LGTM, just one comment for discussion
This PR has been automatically labelled "stale" because it hasn't had recent activity. |
/update-branch |
@DHaussermann Gentle reminder to review the PR |
/update-branch |
Hi @C-Deck, Functionally, this is working a I would expect 👍
I see that you did update the read-me. But Can you please also update the 2 in-product areas as well for the |
This PR has been automatically labelled "stale" because it hasn't had recent activity. |
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 is blocked based on comment above.
Closing due to inactivity |
* [GH-593] Add feature flag for new pull requests - Add new_pulls feature flag - Add check for the flag in postPullRequestEvent - Add check for conflicting flags and refactor the conflicting checks to a helper function * Add feature to README * new_pulls -> pulls_new * pulls_new -> pulls_merged * Additional changes after code review in #612 * errMsg -> testFailureMessage * Revert unnecessary changes --------- Co-authored-by: Clint Decker <clint@tendosystems.com>
Summary
This pull request adds a
new_pulls
feature flag to the subscriptions command to only create notifications for new pull requests. This feature cannot be added ifpulls
is included. There is also some minor refactoring of the checks for conflicting features.Ticket Link
Fixes #593