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

chore(pr-lint): provide output to user in comments #22029

Merged
merged 25 commits into from
Sep 20, 2022

Conversation

TheRealAmazonKendra
Copy link
Contributor

@TheRealAmazonKendra TheRealAmazonKendra commented Sep 14, 2022

The PR linter now provides the output of a failure in the comments. This also updates the node version in the action.

A couple extra notes for reviewer ease:

  • Now that we are writing to the PR, we cannot test with an unauthenticated client so I deleted that section of the documentaion.
  • github-api did not have the apis needed for this change so I switched out its use for @actions/github
  • individual behavior tests no longer throw LinterError. Instead all the errors are collected and displayed to the contributor before the error is thrown.
  • in parser the check on line 14 if (!parsed.scope) would never have been reached because another test threw an error in every case before getting to this line so I've removed it here. It wasn't doing anything and that error case is already covered elsewhere.
  • The many many many changes requested comments in this PR were intentional tests. It's a lot of noise, but useful for showing behavior.
  • Initially I used the wrong token so github-actions was performing the reviews. It's now fixed to be aws-cdk-automation

All Submissions:

Adding new Unconventional Dependencies:

  • This PR adds new unconventional dependencies following the process described here

New Features

  • Have you added the new feature to an integration test?
    • Did you use yarn integ to deploy the infrastructure and generate the snapshot (i.e. yarn integ without --dry-run)?

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

The PR linter now provides the output of a failure in the comments. This also deletes a pr linter file not being used.
@gitpod-io
Copy link

gitpod-io bot commented Sep 14, 2022

@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Sep 14, 2022
@github-actions github-actions bot added the p2 label Sep 14, 2022
@aws-cdk-automation aws-cdk-automation requested a review from a team September 14, 2022 00:30
@TheRealAmazonKendra TheRealAmazonKendra changed the title chore(pr-linter): provide output to user in comments fix(pr-linter): provide output to user in comments Sep 14, 2022
@TheRealAmazonKendra TheRealAmazonKendra marked this pull request as draft September 14, 2022 00:57
@TheRealAmazonKendra
Copy link
Contributor Author

Go away. Not ready yet.

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This PR does not fulfill the following requirement: Fixes must contain a change to an integration test file. PRs must pass status checks before we can provide a meaningful review.

@TheRealAmazonKendra TheRealAmazonKendra changed the title fix(pr-linter): provide output to user in comments chore(pr-linter): provide output to user in comments Sep 14, 2022
@TheRealAmazonKendra
Copy link
Contributor Author

@Mergifyio update

@mergify
Copy link
Contributor

mergify bot commented Sep 14, 2022

update

✅ Branch has been successfully updated

@TheRealAmazonKendra TheRealAmazonKendra added the pr/do-not-merge This PR should not be merged at this time. label Sep 15, 2022
@TheRealAmazonKendra TheRealAmazonKendra marked this pull request as ready for review September 15, 2022 05:48
@TheRealAmazonKendra
Copy link
Contributor Author

TheRealAmazonKendra commented Sep 15, 2022

Adding a do-not-merge tag but making this ready for review because I still need to update documentation but would love feedback on it at this point. As you can see above, when the PR is listed as a fix but no integ tests are added, github actions requests changes.

I'm not totally sure of the error message content, so please let me know what you think of that.

@Naumel, @rix0rrr, @corymhall would love to get your feedback on this.

Note that the local testing instructions had to be deleted in this case because to write to a PR, we can't use an unencrypted client.

@TheRealAmazonKendra
Copy link
Contributor Author

Also, FYI - my next intended step here is to write more validation for the PR title and contents.

Copy link
Contributor

@corymhall corymhall left a comment

Choose a reason for hiding this comment

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

It's awesome that we are finally doing this!

@TheRealAmazonKendra TheRealAmazonKendra changed the title chore(pr-linter): provide output to user in comments fix(pr-linter): provide output to user in comments Sep 17, 2022
Copy link
Collaborator

@aws-cdk-automation aws-cdk-automation left a comment

Choose a reason for hiding this comment

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

The PR Linter fails with the following errors:

❌ Features must contain a change to an integration test file and the resulting snapshot.
❌ Breaking changes should be indicated by starting a line with 'BREAKING CHANGE: ', variations are not allowed. (found: 'BREAKING CHANGE (Not really, manually testing the other linter settings.)').
❌ The title of this PR must specify the module name that the first breaking change should be associated to.

PRs must pass status checks before we can provide a meaningful review.

@TheRealAmazonKendra TheRealAmazonKendra changed the title feat: provide output to user in comments chore(pr-lint): provide output to user in comments Sep 17, 2022
@aws-cdk-automation aws-cdk-automation dismissed their stale review September 17, 2022 02:27

PR updated. Dissmissing previous PRLinter Review.

@TheRealAmazonKendra
Copy link
Contributor Author

@corymhall I didn't mean to delete your comment. I had a bug in the cleanup I was trying to perform but, ultimately, decided that deleting old review comments from our automation didn't actually reduce the noise.

tools/@aws-cdk/prlint/lint.ts Outdated Show resolved Hide resolved
tools/@aws-cdk/prlint/lint.ts Outdated Show resolved Hide resolved
tools/@aws-cdk/prlint/lint.ts Outdated Show resolved Hide resolved
tools/@aws-cdk/prlint/lint.ts Outdated Show resolved Hide resolved
tools/@aws-cdk/prlint/lint.ts Outdated Show resolved Hide resolved
@TheRealAmazonKendra TheRealAmazonKendra removed the pr/do-not-merge This PR should not be merged at this time. label Sep 19, 2022
@TheRealAmazonKendra TheRealAmazonKendra changed the title chore(pr-lint): provide output to user in comments feat(pr-lint): provide output to user in comments Sep 20, 2022
Copy link
Collaborator

@aws-cdk-automation aws-cdk-automation left a comment

Choose a reason for hiding this comment

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

The Pull Request Linter fails with the following errors:

❌ Features must contain a change to an integration test file and the resulting snapshot.

PRs must pass status checks before we can provide a meaningful review.

@TheRealAmazonKendra TheRealAmazonKendra changed the title feat(pr-lint): provide output to user in comments chore(pr-lint): provide output to user in comments Sep 20, 2022
@aws-cdk-automation aws-cdk-automation dismissed their stale review September 20, 2022 02:03

Pull Request updated. Dissmissing previous PRLinter Review.

@corymhall corymhall added the pr-linter/exempt-integ-test The PR linter will not require integ test changes label Sep 20, 2022
@Naumel
Copy link
Contributor

Naumel commented Sep 20, 2022

I'll unfudge the conflict merge.

@Naumel
Copy link
Contributor

Naumel commented Sep 20, 2022

@Mergifyio update

@mergify
Copy link
Contributor

mergify bot commented Sep 20, 2022

update

✅ Branch has been successfully updated

@Naumel
Copy link
Contributor

Naumel commented Sep 20, 2022

@Mergifyio update

@mergify
Copy link
Contributor

mergify bot commented Sep 20, 2022

update

✅ Branch has been successfully updated

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: d30f514
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@mergify
Copy link
Contributor

mergify bot commented Sep 20, 2022

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@mergify mergify bot merged commit 291bf34 into main Sep 20, 2022
@mergify mergify bot deleted the TheRealAmazonKendra/pr-linter branch September 20, 2022 16:47
homakk pushed a commit to homakk/aws-cdk that referenced this pull request Dec 1, 2022
The PR linter now provides the output of a failure in the comments. This also updates the node version in the action.

A couple extra notes for reviewer ease:
- Now that we are writing to the PR, we cannot test with an unauthenticated client so I deleted that section of the documentaion.
- `github-api` did not have the apis needed for this change so I switched out its use for `@actions/github`
- individual behavior tests no longer throw LinterError. Instead all the errors are collected and displayed to the contributor before the error is thrown.
- in `parser` the check on line 14 `if (!parsed.scope)` would never have been reached because another test threw an error in every case before getting to this line so I've removed it here. It wasn't doing anything and that error case is already covered elsewhere.
- The many many many changes requested comments in this PR were intentional tests. It's a lot of noise, but useful for showing behavior.
- Initially I used the wrong token so `github-actions` was performing the reviews. It's now fixed to be `aws-cdk-automation`

----

### All Submissions:

* [ ] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md)

### Adding new Unconventional Dependencies:

* [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies)

### New Features

* [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)?
	* [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)?

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution/core This is a PR that came from AWS. p2 pr-linter/exempt-integ-test The PR linter will not require integ test changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants