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

feat(workflows): create 'test.yml' reusable workflow #13

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

oscard0m
Copy link
Member

@oscard0m oscard0m commented Jul 15, 2022

Resolves #49


Behavior

Before the change?

  • Each of Octokit's repositories has their own copy of test.yml for the CI tests.

After the change?

  • We create a common workflow for CI tests all Octokit's repositories will be able to re-use.

Other information

#49


Additional info

Pull request checklist

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been reviewed and added / updated if needed (for bug fixes / features)
  • Added the appropriate label for the given change

Does this introduce a breaking change?

Please see our docs on breaking changes to help!

  • Yes (Please add the Type: Breaking change label)
  • No

If Yes, what's the impact:

  • N/A

Pull request type

Type: Feature


@oscard0m

This comment was marked as off-topic.

@timrogers timrogers marked this pull request as ready for review July 15, 2022 11:05
Copy link

@timrogers timrogers 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 the time to work on this ❤️

The custom steps is a hard problem. What proportion of repos/workflows have custom steps outside of this framework?

cache: npm
- run: npm ci
- run: npm run test --ignore-scripts # run lint only once
test:

Choose a reason for hiding this comment

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

What do you think of renaming this job to be more indicative?

Suggested change
test:
lint:

This comment was marked as outdated.

Copy link
Contributor

Choose a reason for hiding this comment

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

it's not just linting, we also test things that are independent of node versions, e.g. type tests

Copy link

@timrogers timrogers Jul 16, 2022

Choose a reason for hiding this comment

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

Interesting! Does the lint npm script do all of those things? (I made this suggestion because the only substantive command in the steps is to run the lint script.)

.github/workflows/test.yml Show resolved Hide resolved
@gr2m
Copy link
Contributor

gr2m commented Jul 15, 2022

What approach do we want to follow for custom steps?5

1. Extra custom-test workflows which extend a generic test.yml?
2. Add extra logic to `test.yml` to give support to all the use cases?

I'd prefer option 2., I don't think there should be too many different tasks across the JS octokit/* repositories.

@wolfy1339
Copy link
Member

wolfy1339 commented Jul 15, 2022

octokit/webhooks has many different tasks
The equivalent test job is called npmCi in this case

@gr2m
Copy link
Contributor

gr2m commented Jul 15, 2022

Maybe we only use the reusable test.yml workflow for octokit/*.{js,ts} repositories?

We could also create a setup composite action which only runs actions/checkout and actions/setup-node? It will make our builds a bit slower because the extra action would need to be checked out, but I don't think it'll take a lot.

I think we should be able to add the action.yml file into the .github folder at a path such as actions/setup and then utilize it like this

  - uses: ocotkit/.github/actions/setup

See https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#example-using-a-public-action-in-a-subdirectory

@oscard0m

This comment was marked as outdated.

@gr2m
Copy link
Contributor

gr2m commented Jul 31, 2022

I see some repositories using npm run build

I'd always run it with npm run build --if-present, just like the others. This makes sure that we verify that the dependencies only used for building are tested as part of the CI

  • npm run validate:ts
  • npm run test:typescript
  • npm run test:ts

I'm tending towards test:tsc to be as clear as possible. In other project I use test:tsd for type-only tests using tsd, and in future when I hope me move away from TS source code I could imagine @octokit to use that, too (Example)

But I'm open to anything, it's not set in stone. I do agree we should normalize it across the @octokit repositories though

gr2m added a commit that referenced this pull request Sep 9, 2022
after discussing with @timrogers and @nickfloyd, we decided to remove pinning of actions until we set up #13. The idea of `helpers:pinGitHubActionDigests` is a good one, but in our particular case, the costs of the noise it creates outweighs the benefits
gr2m added a commit that referenced this pull request Sep 9, 2022
after discussing with @timrogers and @nickfloyd, we decided to remove pinning of actions until we set up #13. The idea of `helpers:pinGitHubActionDigests` is a good one, but in our particular case, the costs of the noise it creates outweighs the benefits
@wolfy1339
Copy link
Member

@oscard0m Do you have any updates on this

@gr2m gr2m changed the title feat(workflows): feat(workflows): create 'test.yml' reusable workflow feat(workflows): create 'test.yml' reusable workflow Sep 27, 2022
@oscard0m

This comment was marked as outdated.

@oscard0m

This comment was marked as outdated.

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.

Create 'test.yml' reusable workflow
4 participants