Skip to content
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: add plan_requirements capability #2979

Merged
merged 10 commits into from Jan 21, 2023

Conversation

lyoung-confluent
Copy link
Contributor

@lyoung-confluent lyoung-confluent commented Jan 13, 2023

what

  • Add a new key plan_requirements mirroring the existing apply_requirements and import_requirements keys, but for atlantis plan.

why

  • This can be used to apply restrictions before a plan is executed, such as mandating peer review.

tests

  • I have added tests for all of the new code, mirroring the existing tests for apply/import.

references

@krrrr38
Copy link
Contributor

krrrr38 commented Jan 14, 2023

@lyoung-confluent FYI plan_command_runner may need this impl too https://github.com/krrrr38/atlantis/blob/c6f59309ec5f635f942e519f698397e328cbdde1/server/events/import_command_runner.go#L31-L43

(ctx.PullRequestStatus is mutable value and it requires to be set by command runner now.)

@nitrocode nitrocode changed the title Add plan_requirements capability feat: add plan_requirements capability Jan 16, 2023
@lyoung-confluent lyoung-confluent marked this pull request as ready for review January 18, 2023 22:32
@lyoung-confluent lyoung-confluent requested a review from a team as a code owner January 18, 2023 22:32
@@ -168,6 +171,15 @@ func (p *PlanCommandRunner) run(ctx *command.Context, cmd *CommentCommand) {
baseRepo := ctx.Pull.BaseRepo
pull := ctx.Pull

ctx.PullRequestStatus, err = p.pullReqStatusFetcher.FetchPullStatus(pull)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this needed?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is addressing @krrrr38's comment. For this change we are checking approved and mergeability states during plan and those checks require ctx.PullRequestStatus to be populated. What I saw when testing this without these lines is that the plan was correctly blocked by the approval check, but it would never succeed as the approval status was not updated during a plan run.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This field is useful and we forget to fill-in. So ctx.PullRequestStatus might be better to fill-in at the instance creation timing

ctx := &command.Context{
User: user,
Log: log,
Scope: scope,
Pull: pull,
HeadRepo: headRepo,
PullStatus: status,
Trigger: command.AutoTrigger,
}

But it requires apply_command_runer/import_command_runner change too, so I think it doesn't need it in this PR.

Copy link
Member

@nitrocode nitrocode Jan 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@krrrr38 did your suggestion get implemented? can this comment be resolved? I assume it can since you posted "LGTM" 😄

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The above impl is optional in this PR and not need to do it in this PR, so resolved 👍

server/core/config/valid/global_cfg.go Outdated Show resolved Hide resolved
server/events/command_requirement_handler.go Outdated Show resolved Hide resolved
Copy link
Contributor

@krrrr38 krrrr38 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@nitrocode nitrocode merged commit 5d47eeb into runatlantis:main Jan 21, 2023
@nitrocode
Copy link
Member

Thank you @lyoung-confluent ! Your feature will be available in the dev tag after the build finishes and then officially upon the next release. Please test the feature for any bugs prior to the next release. 🙏

Feel free to propose more changes! Thank you again

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

plan_requirements (similar to apply_requirements)
5 participants