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

chore(ci): include common changes to cache version #433

Merged
merged 15 commits into from
Apr 27, 2022
Merged

Conversation

millotp
Copy link
Collaborator

@millotp millotp commented Apr 26, 2022

🧭 What and Why

Include github actions hash and scripts hash in the cache version, to invalidate everything when those file changes and avoid bumping cache by hand.

The env is a bit quirky, it looks like it's only available to the job itself, not the workflow.
To fight this the cache action will reset the env on every job, making it available to who want's to use it.

🧪 Test

CI

@millotp millotp self-assigned this Apr 26, 2022
@netlify
Copy link

netlify bot commented Apr 26, 2022

Deploy Preview for api-clients-automation canceled.

Name Link
🔨 Latest commit 7858c34
🔍 Latest deploy log https://app.netlify.com/sites/api-clients-automation/deploys/626903777c414b00086c2b30

@algolia-bot
Copy link
Collaborator

algolia-bot commented Apr 26, 2022

✗ The generated branch has been deleted.

If the PR has been merged, you can check the generated code on the main branch.

@millotp millotp requested review from a team, eunjae-lee and damcou and removed request for a team April 26, 2022 14:31
@millotp millotp marked this pull request as draft April 26, 2022 14:45
.github/actions/setup/action.yml Outdated Show resolved Hide resolved
.github/actions/setup/action.yml Outdated Show resolved Hide resolved
damcou
damcou previously approved these changes Apr 26, 2022
@millotp
Copy link
Collaborator Author

millotp commented Apr 26, 2022

Thanks for the review, but this is still in draft I have a few changes to make :)

@damcou
Copy link
Contributor

damcou commented Apr 26, 2022

Thanks for the review, but this is still in draft I have a few changes to make :)

Yeah sorry I saw that it was still a draft afterwards :s .

Comment on lines 129 to 132
const hash = (await hashElement('.', check.hashOptions)).hash.replace(
/[=/]/g,
''
);
Copy link
Member

Choose a reason for hiding this comment

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

Does it default to base64? Can't we ask for hexadecimal so we don't have to handle it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ah good idea it can be hex

@millotp millotp requested review from damcou and shortcuts April 26, 2022 18:17
@millotp millotp marked this pull request as ready for review April 26, 2022 18:17
Copy link
Member

@shortcuts shortcuts left a comment

Choose a reason for hiding this comment

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

Great addition!!

Comment on lines +19 to +24
const VARIABLES_TO_CHECK: Array<{
name: string;
path: string[];
needHash?: boolean;
hashOptions?: HashElementOptions;
}> = [
Copy link
Member

Choose a reason for hiding this comment

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

Since it's not used elsewhere the inferred type is enough

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it not inferring the type of hashOptions on it's own, this is needed to have autocompletion

Copy link
Member

Choose a reason for hiding this comment

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

Oh :(

shortcuts
shortcuts previously approved these changes Apr 26, 2022
Copy link
Member

@shortcuts shortcuts left a comment

Choose a reason for hiding this comment

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

gg!!!

@millotp millotp enabled auto-merge (squash) April 26, 2022 18:46
@millotp
Copy link
Collaborator Author

millotp commented Apr 26, 2022

jk the ci is broken again

damcou
damcou previously approved these changes Apr 27, 2022
@shortcuts
Copy link
Member

it looks like the CTS step restore cache is missing the env.CACHE_VERSION: 8.0.10.0.8--563cdb62f6630673b662c6b205d8315bc048ee68a8a1c2e35110b10c07458c9b

@millotp millotp dismissed stale reviews from damcou and shortcuts via d74e52f April 27, 2022 08:28
Comment on lines 22 to 23
echo "CACHE_VERSION=$(< .github/.cache_version)" >> $GITHUB_ENV
echo "CACHE_COMMON_HASH=${{ inputs.cache_hash }}" >> $GITHUB_ENV
Copy link
Contributor

Choose a reason for hiding this comment

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

what if we concatenate .cache_version and cash_hash and assign to CACHE_VERSION?

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 tried that at first but CACHE_VERSION only is needed for yarn cache, because we need yarn to build CACHE_COMMON_HASH.
Actually it could be in it's own variable, that would make more sense, but it still needs to be passed around everywhere

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah okay, it makes sense. Tough problem 😅

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it's much cleaner like that thank you :)

Copy link
Contributor

@eunjae-lee eunjae-lee left a comment

Choose a reason for hiding this comment

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

Looks good to me as long as it passes all the CI checks.

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.

5 participants