Added UnlockedApplyRequirement to support 🔒 and 🔓 emoji for atlantis apply #90
Conversation
Aayyush
added
enhancement
server/events/yaml/raw/project.go
Outdated
| if r != ApprovedApplyRequirement && r != MergeableApplyRequirement && r != UnDivergedApplyRequirement && r != UnLockedApplyRequirement { | ||
| return fmt.Errorf("%q is not a valid apply_requirement, only %q, %q, %q and %q are supported", r, ApprovedApplyRequirement, MergeableApplyRequirement, UnDivergedApplyRequirement, UnLockedApplyRequirement) |
There was a problem hiding this comment.
I feel like this can be optimized instead of growing a large if statement haha
There was a problem hiding this comment.
Yeah that sounds like a better approach 😅!
server/events/models/models.go
Outdated
| @@ -423,7 +423,7 @@ func (p ProjectCommandContext) ProjectCloneDir() string { | |||
| // SetScope sets the scope of the stats object field. Note: we deliberately set this on the value | |||
| // instead of a pointer since we want scopes to mirror our function stack | |||
| func (p ProjectCommandContext) SetScope(scope string) { | |||
| p.Scope = p.Scope.Scope(scope) | |||
| p.Scope = p.Scope.Scope(scope) //nolint | |||
There was a problem hiding this comment.
Why'd you add this tag?
There was a problem hiding this comment.
I'm getting this warning without the tag ineffective assignment to field ProjectCommandContext.Scope (SA4005)go-staticcheck. I looked up the error code SA4005 and this is what I found SA4005 – Field assignment that will never be observed. Did you mean to use a pointer receiver. I'm not sure what it means.
There was a problem hiding this comment.
Looks like it's passing all the tests without the //nolint tag as well. So, I have removed it.
There was a problem hiding this comment.
oh i think you found a bug of mine 😅 . I can address in another PR though.
cmd/server.go
Outdated
| @@ -87,6 +87,7 @@ const ( | |||
| RepoWhitelistFlag = "repo-whitelist" | |||
| RepoAllowlistFlag = "repo-allowlist" | |||
| RequireApprovalFlag = "require-approval" | |||
| RequireUnlockedFlag = "require-unlocked" | |||
There was a problem hiding this comment.
Maybe let's be specific so it's not confusing for future readers and let's call it: RequireSQUnlockedFlag
server/events/yaml/raw/project.go
Outdated
| isValid := false | ||
| for _, applyRequirement := range applyRequirements { | ||
| if req == applyRequirement { | ||
| isValid = true |
There was a problem hiding this comment.
why not just use a map instead of a list, this would become one line of code then?
There was a problem hiding this comment.
Hmm that's true! I'll change it to a map.
There was a problem hiding this comment.
Looks like using a map makes the buildValidApplyRequirementsString() unpredictable causing the tests to fail occasionally. Should I just revert back to using a slice or should I change how we're generating the test statement?
There was a problem hiding this comment.
You can probably just account for that failure by sorting the result.
server/events/yaml/raw/project.go
Outdated
| var returnString strings.Builder | ||
| counter := 0 | ||
| for _, applyReq := range applyRequirementsList { | ||
| if counter == len(applyRequirements)-1 { | ||
| returnString.WriteString(fmt.Sprintf("and %q", applyReq)) | ||
| break | ||
| } | ||
| returnString.WriteString(fmt.Sprintf("%q, ", applyReq)) | ||
| counter++ | ||
| } | ||
| return returnString.String() |
There was a problem hiding this comment.
lets just replace this logic in favor of: strings.Join(applyRequirementsList, ",") Also we can make the result of this function a global var since this never changes after startup.
The errorf statement above can then be "%q is not a valid apply_requirement, supported apply requirements are: ..."
No description provided.