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

ci: only git fetch HEAD unless more is needed #5938

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

ci: only git fetch HEAD unless more is needed #5938

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1f3df38
test: fix test accidentally relying on CI env
serhalp Dec 11, 2024
b9daa55
ci: only git fetch minimum necessary
serhalp Dec 11, 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: 2 additions & 1 deletion .github/workflows/pre-release.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,8 @@ jobs:
steps:
- uses: actions/checkout@v4
with:
fetch-depth: 0
# Needed for git tag updates below
fetch-tags: true
- uses: actions/setup-node@v4
with:
node-version: '*'
Expand Down
2 changes: 0 additions & 2 deletions .github/workflows/typescript-nudge.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,6 @@ jobs:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v4
with:
fetch-depth: 0

- name: 🔎 Get changed JavaScript files
id: changed-javascript-files
Expand Down
6 changes: 4 additions & 2 deletions .github/workflows/workflow.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,9 @@ jobs:
if: ${{ startsWith(github.head_ref, 'release-') }}
- uses: actions/checkout@v4
with:
# Needed for `nrwl/nx-set-shas` Action, ultimately for `nx affected`
fetch-depth: 0
fetch-tags: false
if: ${{ !steps.release-check.outputs.IS_RELEASE }}
- uses: nrwl/nx-set-shas@v3
if: ${{ !steps.release-check.outputs.IS_RELEASE }}
Expand Down Expand Up @@ -75,7 +77,9 @@ jobs:
if: ${{ startsWith(github.head_ref, 'release-') }}
- uses: actions/checkout@v4
with:
# Needed for `nrwl/nx-set-shas` Action, ultimately for `nx affected`
fetch-depth: 0
fetch-tags: false
if: ${{ !steps.release-check.outputs.IS_RELEASE }}
- uses: nrwl/nx-set-shas@v3
if: ${{ !steps.release-check.outputs.IS_RELEASE }}
Expand Down Expand Up @@ -183,8 +187,6 @@ jobs:
if: ${{ startsWith(github.head_ref, 'release-') }}
- name: Git checkout
uses: actions/checkout@v4
with:
fetch-depth: 0
if: ${{ !steps.release-check.outputs.IS_RELEASE }}
- name: setup pnpm/yarn
run: corepack enable
Expand Down
5 changes: 4 additions & 1 deletion packages/build/tests/core/tests.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -287,7 +287,10 @@ test('--branch', async (t) => {
})

test('--baseRelDir', async (t) => {
const output = await new Fixture('./fixtures/basereldir').withFlags({ baseRelDir: false }).runWithConfig()
const output = await new Fixture('./fixtures/basereldir')
.withFlags({ baseRelDir: false })
.withEnv({ COMMIT_REF: 'abcdefabcdefabcdefabcdef', CACHED_COMMIT_REF: undefined })
Copy link
Contributor

Choose a reason for hiding this comment

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

Learning: what's this change for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I explained this in the separate commit but not in the PR!

This is one of those funny things about testing a build system on another build system :). It seems this snapshot test was unintentionally relying on some CI env vars related to the git checkout of the netlify/build repo, when it was intending to be testing something related to a test fixture repo's git env vars.

When I changed the git checkout setup in our CI, it changed this snapshot test's result a bit. So I updated the test to explicitly pass the env it's expecting.

I would imagine if we changed all these tests to not inherit the outer env at all it would break a lot of things...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

... or at least I thought I did but it didn't actually fix it. I don't think the undefined thing is working 😢.

.runWithConfig()
t.snapshot(normalizeOutput(output))
})

Expand Down
Loading