-
Notifications
You must be signed in to change notification settings - Fork 31
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: compact format for if, then and actions #824
Conversation
AI-Generated Summary: This pull request contains two patches:
|
Reviewpad Report
|
/reviewpad summarize |
AI-Generated Summary: This pull request contains two patches that introduce support for compact if-then actions and adds test coverage for them. The first patch modifies the The second patch adds new test cases to the |
engine/inline_rules_normalizer.go
Outdated
processedActions, err := processCompactActions(rawAction) | ||
if err != nil { | ||
return nil, err | ||
} |
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.
Is this required? From my understanding, actions can either be a string or a list. In the last case, we can iterate and append. Does running processCompactActions
is necessary?
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.
we are gonna have to check the list elements are a string right, instead of repeating that in the loop I figured we could reuse it
engine/testdata/loader/process/reviewpad_with_multiple_pipelines_with_inline_actions.yml
Outdated
Show resolved
Hide resolved
|
||
actions = append(actions, processedActions...) | ||
} | ||
case nil: |
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.
Why nil
? What is the use case for nil
? In this case shouldn't we return an empty list, i.e. actions
?
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.
that's in case the workflow doesn't have any actions but only extra actions associated with the rules
switch action := nonNormalizedActions.(type) { | ||
case string: | ||
actions = append(actions, action) | ||
case []any: |
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.
Shouldn't this be []string
?
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.
since it's an interface{}
type we are processing the type will be []any
instead of []string
|
||
switch r := rawRule.(type) { | ||
case string: | ||
rule = decodeRule(r) |
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.
If my understanding is correct being string can mean the name of a specified rule under rules
, which means that this decode is not correct. It works because on line 136 we look for all rules and we will end up with the correct spec.
We don't need to correct this since it's working and we will be addressing this issue later.
Just wanted to create awareness.
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.
Yes, this is just creating a rule structure using the spec as a name and we verify it doesn't exist at a later stage, it was a way of avoiding having duplicate rules if the same spec is repated
AI-Generated Summary: This pull request includes 3 commits that add support for compact if-then actions, provide test coverage for those actions, and address reviews. The changes involve modifications to the
|
📈 Pull Request Metrics💻 Coding Time: 12 minutes |
Description
Introduces a compact form for
if
,then
andactions
fields in reviewpad file.Related issue
Closes reviewpad/raas-common#103
Type of change
New feature (non-breaking change which adds functionality)
How was this tested?
Manual and Unit tests.
Checklist
task check -f
and have no issuesCode review and merge strategy (ship/show/ask)
Ask: this pull request requires a code review before merge