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

ci(v2): remove duplicated lint jobs/steps #5562

Merged
merged 2 commits into from
Aug 2, 2024
Merged

Conversation

galargh
Copy link
Member

@galargh galargh commented Aug 1, 2024

  • Because this PR includes a bug fix, relevant tests have been included.
  • Because this PR includes a new feature, the change was previously discussed on an Issue or with someone from the team.
  • I didn't do anything of this.

This PR is a subset of #5301.

In this PR, I minimise the number of places where we perform linting to limit duplication. In particular:

  1. I changed the trigger of the Lint workflow to pull_request_target and added a second job that comments on a PR when needed.
  2. I removed Comment on lint failure workflow because it is duplicated by the Lint workflow after the changes.
  3. I removed the linting step from the Check Docs Site workflow because it is duplicated in the Lint workflow.

Copy link

vercel bot commented Aug 1, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
hardhat ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 2, 2024 3:31pm

Copy link

changeset-bot bot commented Aug 1, 2024

⚠️ No Changeset found

Latest commit: a452682

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@github-actions github-actions bot added the status:ready This issue is ready to be worked on label Aug 1, 2024
steps:
- uses: actions/checkout@v4
with:
Copy link
Member

Choose a reason for hiding this comment

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

Out of curiosity: what's the deal behind this double checkout?

Is the first one for pushes to main and the other one checking out the merge commit of a pr?

Maybe leaving a comment would be good for future readers.

Copy link
Member Author

Choose a reason for hiding this comment

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

I missed an if here. The first checkout should only be executed if we're not running in the pull request context. Thanks for catching this 🙇

Copy link
Member Author

Choose a reason for hiding this comment

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

FYI, after resolving the conflicts I had to restore 2 checkouts when we're running on pull_request_target. This is so that we can run lint on the code from the pull request but get the code of the setup-env action from the base branch.

I added the following comment above the first checkout so that this bit of context is not lost:

      # NOTE: When running on a pull_request_target, we're checking out the repo
      # twice. The first checkout is for the base branch of the PR. The second,
      # for the merge commit. We do this to be able to use the `setup-env`
      # action that's on the base branch.

@galargh galargh force-pushed the galargh/github-actions-lint branch from 8695e07 to 848980a Compare August 2, 2024 11:20
@galargh galargh changed the base branch from v-next to main August 2, 2024 11:21
@galargh galargh changed the title ci: remove duplicated lint jobs/steps ci: remove duplicated lint jobs/steps (v2) Aug 2, 2024
@galargh galargh changed the title ci: remove duplicated lint jobs/steps (v2) ci(v2): remove duplicated lint jobs/steps Aug 2, 2024
@galargh galargh force-pushed the galargh/github-actions-lint branch from 848980a to 38bb49e Compare August 2, 2024 11:44
@galargh galargh added the no changeset needed This PR doesn't require a changeset label Aug 2, 2024
@galargh galargh marked this pull request as ready for review August 2, 2024 11:51
@galargh galargh merged commit 05664bc into main Aug 2, 2024
10 checks passed
@galargh galargh deleted the galargh/github-actions-lint branch August 2, 2024 16:02
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 1, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
no changeset needed This PR doesn't require a changeset status:ready This issue is ready to be worked on
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants