-
Notifications
You must be signed in to change notification settings - Fork 11
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
Add support for AllowEmptyRun #38
Conversation
pkg/comment_actions/parsing.go
Outdated
existedOnce := false | ||
for _, v := range s { | ||
if v == str { | ||
existedOnce = 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.
If all we care about is the existence of the string, we might want to break
the loop here and immediately return, ie
func Contains(s []string, str string) bool {
for _, v := range s {
if v == str {
return true
}
}
return false
}
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.
@polyrain thank you for your feedback, I will removing all these and you can refer to #38 (comment)
pkg/comment_actions/parsing.go
Outdated
|
||
if len(words) == 0 { | ||
return nil, ErrNoNotePassed | ||
} | ||
|
||
if len(words)%2 != 0 { | ||
isEscaped := false | ||
for _, a := range escapeKeyword { |
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 can probably drop this and replace it with something like:
if len(words)%2 != 0 && (slices.Contains(words, "-e") || slices.Contains(words, "--allow_empty_run")) { ... }
As of Go 1.21 this is now part of the standard library. Example: https://go.dev/play/p/cxVe9DRKfXQ
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 think we can just drop this now since we check if there's any commands and then we also validate the flags https://github.com/zapier/tfbuddy/blob/main/pkg/comment_actions/parsing.go#L49. We should also update the unit test for this function to ensure that it works as expected wth valid and invalid flags passed.
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.
will remove this checking
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.
Looks good, minor comments
pkg/comment_actions/parsing.go
Outdated
|
||
if len(words) == 0 { | ||
return nil, ErrNoNotePassed | ||
} | ||
|
||
if len(words)%2 != 0 { | ||
isEscaped := false | ||
for _, a := range escapeKeyword { |
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 think we can just drop this now since we check if there's any commands and then we also validate the flags https://github.com/zapier/tfbuddy/blob/main/pkg/comment_actions/parsing.go#L49. We should also update the unit test for this function to ensure that it works as expected wth valid and invalid flags passed.
Signed-off-by: Ken Ng <[email protected]>
Signed-off-by: Ken Ng <[email protected]>
Signed-off-by: Ken Ng <[email protected]>
Signed-off-by: Ken Ng <[email protected]>
Signed-off-by: Ken Ng <[email protected]>
Signed-off-by: Ken Ng <[email protected]>
Adding support to
AllowEmptyApply
via flag-e
see the issue from #18