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.

Sign up for GitHub

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

BREAKING: hoisted resources out of data: #11

Merged
merged 5 commits into from
Mar 6, 2024
Merged

BREAKING: hoisted resources out of data: #11

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
097854e
BREAKING: hoisted resources out of data:
philsturgeon Mar 6, 2024
0e40871
fix linting errors
philsturgeon Mar 6, 2024
fc8cae8
attempt to remove double checks
philsturgeon Mar 6, 2024
de463b6
reverted workflows to show it doing double work
philsturgeon Mar 6, 2024
a40da94
Switch to CC-BY-SA-4.0
philsturgeon Mar 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 0 additions & 3 deletions .github/workflows/bump.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,6 @@ permissions:
contents: read
pull-requests: write
on:
push:
branches:
- main
Copy link
Collaborator

Choose a reason for hiding this comment

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

why do you remove the workflow from the push on main branch. It's still needed when we merge in a PR into main branch, to deploy the API document to Bump.sh :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

With the current setup it was doing all checks twice, and as well as looking untidy there's a carbon footprint to doing everything twice. If you can help me get this right I'd appreciate it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's kind of doing checks twice.

However, running workflows on the HEAD of a pull request is slightly different than running workflows on the HEAD of the target branch (here always main).

That's because the merged PR can have difference with the head of the PR (if there's changes on the main while the PR is being developed)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't mean "doing it twice" like once in the PR and once again on main.

I mean it was literally doing it twice on the PR. This happens when you do a first push because both the push event and the pull_request event are triggered, so its literally running all the checks simultaniously twice, once for push and once for pull_request.

This is a documented thing, and its wasting compute resources: https://github.com/orgs/community/discussions/26940

I've reverted the change here, but I would like to not only fix it in this repo, but in the sample Bump.sh workflow in documentation.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hm 🤔, I can't see any duplicate jobs on the commits of the HEAD of this PR:

bc97140
Capture d’écran du 2024-03-06 19-38-43

There's only:

  • one Lint job (the second check --> Linter OPENAPI is a created in the Lint job by the super-linter gh action)
  • only one Check API diff job.

The description of the push target in the GH action workflow file:

on:
  push:
    - main

Means it will only trigger a workflow on push on the main branch (so not on pushed on any other branches, i.e. pull request branches):

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The first push will have two lots of checks as you've opened a pull request.

The second push will have only one lot of checks because the pull request is old now.

I'll take a screenshot next time this happens.

Copy link
Collaborator

Choose a reason for hiding this comment

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

There's no reason for a duplicate job with the current workflow definition file, sorry to insist!

Here's a demo with a newly created PR with a single push on that PR branch: #13

There's a single Lint workflow and a single Check & Deploy API documentation workflow` running on the repository:
Capture d’écran du 2024-03-06 20-15-35

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I saw the duplicates. I saw two of everything. I should have taken a screenshot, but I will do next time it happens.

pull_request:
branches:
- main
Expand Down
6 changes: 2 additions & 4 deletions .github/workflows/lint.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,7 @@
name: Lint

on: # yamllint disable-line rule:truthy
pull_request: {}
push:
branches: [main]
philsturgeon marked this conversation as resolved.
Show resolved Hide resolved
on:
pull_request:
philsturgeon marked this conversation as resolved.
Show resolved Hide resolved
paths-ignore:
- 'README.md'

Expand Down
2 changes: 2 additions & 0 deletions .spectral.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,3 +7,5 @@ rules:
owasp:api4:2019-array-limit: false
owasp:api4:2019-string-limit: false
owasp:api4:2019-string-restricted: false
owasp:api4:2019-integer-limit: false
owasp:api4:2019-integer-format: false
Loading
Loading