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

docs: document approve_policies command in comment_parser #1886

Merged
merged 2 commits into from
Nov 17, 2021

Conversation

dupuy26
Copy link
Contributor

@dupuy26 dupuy26 commented Nov 8, 2021

The approve_policies command was added with policy checking but was
never included in the command parser help.

Add an explicit mention of the approve_policies command in the error
message on policy checking failure.

Also update the comments in the file to reflect all of the commands.

@dupuy26 dupuy26 requested a review from a team as a code owner November 8, 2021 11:27
@snuggie12
Copy link

if it's not too much of a stretch you might want to add the word to the footer of the comment when there are failures. Saves reviewers time looking it up

@nishkrishnan
Copy link
Contributor

Thanks for doing this! Looks like there's some tests failing though! If you could fix those we can get this in.

@dupuy26 dupuy26 changed the title docs: document approve_policies command in help docs: document approve_policies command in comment_parser Nov 15, 2021
The `approve_policies` command was added with policy checking but was
never included in the command parser help.

Also update the comments in the file to reflect all of the commands.
@dupuy26
Copy link
Contributor Author

dupuy26 commented Nov 15, 2021

if it's not too much of a stretch you might want to add the word to the footer of the comment when there are failures. Saves reviewers time looking it up

Excellent idea – I have updated that text to include the specific command.

@dupuy26 dupuy26 force-pushed the patch-2 branch 3 times, most recently from cbe44ce to c54e1f7 Compare November 15, 2021 14:01
Include the specific command `approve_policies` in the message.
@dupuy26
Copy link
Contributor Author

dupuy26 commented Nov 15, 2021

Looks like there's some tests failing though! If you could fix those we can get this in.

Now passing all tests for both modified help message and policy check failure error.

Copy link
Member

@chenrui333 chenrui333 left a comment

Choose a reason for hiding this comment

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

lgtm

@chenrui333 chenrui333 merged commit dc4cc2f into runatlantis:master Nov 17, 2021
@dupuy26 dupuy26 deleted the patch-2 branch December 17, 2021 15:28
krrrr38 pushed a commit to krrrr38/atlantis that referenced this pull request Dec 16, 2022
…s#1886)

* docs: document approve_policies command in help

The `approve_policies` command was added with policy checking but was
never included in the command parser help.

Also update the comments in the file to reflect all of the commands.

* feat: Better message on policy failure

Include the specific command `approve_policies` in the message.
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.

5 participants