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

🏗 🐛 ❄️ always check forbidden terms #34897

Merged
merged 6 commits into from
Jun 16, 2021

Conversation

rileyajones
Copy link
Contributor

#34887 demonstrated that the forbidden-terms file always needs to be checked by removing forbidden terms from files on the allowlist. Because the we run amp lint --local_changes in CI now the checks passed but then started failing on main.

Checks run from bad PR
https://app.circleci.com/pipelines/github/ampproject/amphtml/11572/workflows/fa5345cd-0ebe-4663-b8a1-f5905a1b63da/jobs/183106
image

Future checks run on main
https://app.circleci.com/pipelines/github/ampproject/amphtml/11572/workflows/fa5345cd-0ebe-4663-b8a1-f5905a1b63da/jobs/183106
image

@amp-owners-bot
Copy link

amp-owners-bot bot commented Jun 16, 2021

Hey @rsimha! These files were changed:

build-system/common/utils.js
build-system/pr-check/build-targets.js

@rileyajones rileyajones enabled auto-merge (squash) June 16, 2021 21:03
Copy link
Contributor

@rsimha rsimha left a comment

Choose a reason for hiding this comment

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

Thanks for taking care of this! LGTM with a few minor suggestions.

build-system/common/utils.js Outdated Show resolved Hide resolved
build-system/common/utils.js Outdated Show resolved Hide resolved
build-system/common/utils.js Show resolved Hide resolved
@rileyajones rileyajones disabled auto-merge June 16, 2021 21:10
@rileyajones rileyajones force-pushed the forbidden-terms-fix branch 2 times, most recently from 50a752d to 24089a5 Compare June 16, 2021 21:21
@rileyajones rileyajones force-pushed the forbidden-terms-fix branch from 24089a5 to 3797520 Compare June 16, 2021 21:31
build-system/common/utils.js Outdated Show resolved Hide resolved
build-system/pr-check/build-targets.js Outdated Show resolved Hide resolved
build-system/common/utils.js Outdated Show resolved Hide resolved
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.

3 participants