Skip to content

CI: inline the move-caches script into the action #12213

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

Merged
merged 2 commits into from
Feb 16, 2023

Conversation

erik-krogh
Copy link
Contributor

@erik-krogh erik-krogh commented Feb 16, 2023

That way the action also works when used by another repository that includes github/codeql as a submodule.

And a drive-by change that enforces that we always run QL-for-QL.

@erik-krogh erik-krogh marked this pull request as ready for review February 16, 2023 13:58
@erik-krogh erik-krogh requested a review from a team as a code owner February 16, 2023 13:58
@kaeluka
Copy link

kaeluka commented Feb 16, 2023

That way the action also works when used by another repository that includes github/codeql as a submodule.

How is that currently broken?

@erik-krogh
Copy link
Contributor Author

How is that currently broken?

The $GITHUB_WORKSPACE path references the "root" repository.
So the path to move-caches.js didn't work when github/codeql is within a submodule.
I didn't find a good way to get the path to the current workflow file (well, I didn't find anything that worked for composite workflows).
So inlining the script seemed like a good solution.

Copy link

@kaeluka kaeluka left a comment

Choose a reason for hiding this comment

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

left two minor comments

Comment on lines +51 to +57
// # Move all the existing cache into another folder, so we only preserve the cache for the current queries.
// mkdir -p ${COMBINED_CACHE_DIR}
// rm -f **/.cache/{lock,size} # -f to avoid errors if the cache is empty.
// # copy the contents of the .cache folders into the combined cache folder.
// cp -r **/.cache/* ${COMBINED_CACHE_DIR}/ || : # ignore missing files
// # clean up the .cache folders
// rm -rf **/.cache/*
Copy link

Choose a reason for hiding this comment

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

delete?

Suggested change
// # Move all the existing cache into another folder, so we only preserve the cache for the current queries.
// mkdir -p ${COMBINED_CACHE_DIR}
// rm -f **/.cache/{lock,size} # -f to avoid errors if the cache is empty.
// # copy the contents of the .cache folders into the combined cache folder.
// cp -r **/.cache/* ${COMBINED_CACHE_DIR}/ || : # ignore missing files
// # clean up the .cache folders
// rm -rf **/.cache/*

@@ -5,13 +5,6 @@ on:
branches: [main]
pull_request:
branches: [main]
paths:
Copy link

Choose a reason for hiding this comment

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

why remove these paths?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wanted to trigger QL-for-QL on this PR.
The simplest approach was just to always run QL-for-QL.

@erik-krogh erik-krogh merged commit 2b529fb into main Feb 16, 2023
@erik-krogh erik-krogh deleted the erik-krogh/patch-test branch February 16, 2023 15:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants