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

build: update actions file #39871

Closed
wants to merge 3 commits into from
Closed

Conversation

Mesteery
Copy link
Contributor

Reformat actions to maintain readability and consistency
between different actions.
Update some used actions (e.g. actions/upload-artifact).
Rename misc action.
Add more (complete) path-ignores.

@nodejs-github-bot nodejs-github-bot added the meta Issues and PRs related to the general management of the project. label Aug 24, 2021
- main
- v[0-9]+.x-staging
- v[0-9]+.x
branches: [master, main, 'v[0-9]+.x-staging', 'v[0-9]+.x']
Copy link
Member

Choose a reason for hiding this comment

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

I find multiline arrays easier to read (and also to review when elements are added/removed)

Copy link
Contributor Author

@Mesteery Mesteery Aug 25, 2021

Choose a reason for hiding this comment

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

The line just above is (and was) a single line areay, and the two lines have almost the same size. Personally I find that when the inline version is short it's easier to read, otherwise I leave it multiline, like in coverage-linux.yml.
But if you prefer multiline arrays, there is no problem, I would put all arrays in multiline.

Copy link
Member

@targos targos Aug 25, 2021

Choose a reason for hiding this comment

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

yeah, I didn't put my comment on the best line. I don't feel strongly about the types and branches fields, but I would like at least the paths-ignore to stay multiline

- v[0-9]+.x-staging
- v[0-9]+.x
branches: [master, main, canary, 'v[0-9]+.x-staging', 'v[0-9]+.x']
paths-ignore: ['**.md', doc/**, AUTHORS, .mailmap, .github/**, '!.github/workflows/test-linux.yml']
Copy link
Member

Choose a reason for hiding this comment

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

We want to run at least one build for doc changes -- at least one test checks for consistency between documentation and run time (hence needing a built node binary) behaviour:

Copy link
Member

Choose a reason for hiding this comment

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

I think there are also addon tests that are built from the docs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

-     paths-ignore: ['**.md', doc/**, AUTHORS, .mailmap, .github/**, '!.github/workflows/test-linux.yml']
+     paths-ignore: ['AUTHORS, .mailmap, .github/**, '!.github/workflows/test-linux.yml']

?

Reformat actions to maintain readability and consistency
between different actions.
Update some used actions (e.g. `actions/upload-artifact`).
Rename misc action.
Add more (complete) `path-ignores`.
Use GitHub CLI when possible.
@Mesteery
Copy link
Contributor Author

Closing in favor of #40985, #40986, #40987, and #40988.

@Mesteery Mesteery closed this Nov 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
meta Issues and PRs related to the general management of the project.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants