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

[DT-1250][risk=no] Check for lint in the files that have changed between this PR and the destination branch. #2791

Merged
merged 17 commits into from
Feb 19, 2025

Conversation

otchet-broad
Copy link
Contributor

@otchet-broad otchet-broad commented Feb 18, 2025

Addresses

https://broadworkbench.atlassian.net/browse/DT-1250

Summary

Lint checking on only files that have changed in this PR.

Screenshots

When there's no lint, the action passes as expected. The file in this case is excluded because of linter configuration.
image

When there's lint, we get a failure:
image

When the lint is fixed:
image

Lint history tracked in PR history:
image


Have you read Terra's Contributing Guide lately? If not, do that first.

  • Label PR with a Jira ticket number and include a link to the ticket
  • Label PR with a security risk modifier [no, low, medium, high]
  • PR describes scope of changes
  • Get a minimum of one thumbs worth of review, preferably two if enough team members are available
  • Get PO sign-off for all non-trivial UI or workflow changes
  • Verify all tests go green
  • Test this change deployed correctly and works on dev environment after deployment

- uses: actions/checkout@v4
with:
persist-credentials: false
fetch-depth: 0
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Required to get the develop branch.

@otchet-broad otchet-broad marked this pull request as ready for review February 18, 2025 19:09
@otchet-broad otchet-broad requested a review from a team as a code owner February 18, 2025 19:09
@otchet-broad otchet-broad requested review from snf2ye, s-rubenstein, rushtong and fboulnois and removed request for a team February 18, 2025 19:09
Copy link
Contributor

@rushtong rushtong left a comment

Choose a reason for hiding this comment

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

👍🏽

Copy link
Contributor

@fboulnois fboulnois left a comment

Choose a reason for hiding this comment

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

Looks good, one suggestion below:

otchet-broad and others added 3 commits February 18, 2025 14:55
Use an environment variable instead of directly referencing the template.

Co-authored-by: fboulnois <[email protected]>
@fboulnois
Copy link
Contributor

fboulnois commented Feb 18, 2025

I see why the failure is happening, GITHUB_REF is already an environment variable: https://docs.github.com/en/actions/writing-workflows/choosing-what-your-workflow-does/store-information-in-variables

I would switch it to GITHUB_CUSTOM_EVENT_REF or something similar. You may also be able to use GITHUB_BASE_REF directly, without using the env block.

with:
persist-credentials: false
fetch-depth: 0
- run: echo "NODE_VERSION=$(cat Dockerfile | awk 'NR==2 {gsub(":","@",$2); print $2}' | awk '{split($0, array, "@"); print array[2]}')" >> $GITHUB_ENV
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice!

Copy link
Contributor

@rushtong rushtong left a comment

Choose a reason for hiding this comment

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

👍🏽

Copy link
Contributor

@fboulnois fboulnois left a comment

Choose a reason for hiding this comment

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

🎉

@otchet-broad otchet-broad merged commit 6311d8c into develop Feb 19, 2025
10 checks passed
@otchet-broad otchet-broad deleted the otchet/DT-1250-lint-pr-check branch February 19, 2025 15:17
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