-
Notifications
You must be signed in to change notification settings - Fork 127
Custom properties predicate #1054
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
Custom properties predicate #1054
Conversation
Generate changelog in
|
|
@bluekeyes hello, could you let us know if this implementation matches the design as suggested at #1029 (comment), and if any other modifications are needed? |
|
Sorry, I was unavailable last week, but this is on my list to review this week. Thanks for submitting the PR! |
bluekeyes
left a comment
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 submitting this! I had a few comments related to how the details messages are produced, but I think the core logic of the predicates all looks good.
| ValuePhrase: "specified custom properties", | ||
| Values: formatCustomProperties(customProperties), | ||
| ConditionPhrase: "have a non-null value", | ||
| ConditionValues: pred, | ||
| Satisfied: true, |
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.
The current structure for these messages will render a bit awkwardly. I think there are two options to improve it:
-
Use the predicate to populate the values and omit the condition values:
ValuePhrase: "custom properties", Values: pred, ConditionPhrase: "exist",
This is the simplest, but does not show the actual properties that are set on the repository.
-
Adjust the messages so that you can show all of the actual properties:
ValuePhrase: "custom properties", Values: formatCustomProperties(customProperties), ConditionPhrase: "contain", ConditionValues: pred,
I prefer the first option. I think the message is clearer and the actual values of the properties are not as important when the predicate check only if they are present or not.
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.
I chose the second option in baf70ef, because it might be a bit confusing to debug without the current values.
If the first option is strongly preferred, I can switch to it.
| ValuePhrase: "specified custom properties", | ||
| Values: formatCustomProperties(customProperties), | ||
| ReverseSkipPhrase: true, | ||
| ConditionPhrase: "have a non-null value", | ||
| ConditionValues: pred, | ||
| Satisfied: true, |
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 message should also be updated in the same way as the message for CustomPropertyIsNotNull, but adding ReverseSkipPhrase: true.
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.
I chose the second option in baf70ef, because it might be a bit confusing to debug without the current values.
If the first option is strongly preferred, I can switch to it.
✅ Successfully generated changelog entry!What happened?Your changelog entries have been stored in the database as part of our migration to ChangelogV3. Need to regenerate?Simply interact with the changelog bot comment again to regenerate these entries. |
|
Thank you for the review. I will fix the conflict, address the issue and then request another review. |
a390b8b to
ea7bbfc
Compare
bluekeyes
left a comment
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! We have some other pending features that I need to review and test, but I'll try to make a release with these new predicates soon.
Before this PR
Custom properties are not supported in predicates.
After this PR
Custom properties are supported in predicates, implemented in the way as proposed in #1029 (comment).
Possible downsides?
New predicate are not active unless they are used in the policy, so there should be no negative impact.
Screenshots
Some screenshots when testing the feature to cover the "testing" of
ConditionPhraseandReverseSkipPhrasethat is not covered bypolicy/predicate/custom_properties_test.gomatch
nomatch
empty