-
Notifications
You must be signed in to change notification settings - Fork 66
move utils.sh to scripts/utils.sh + also update script to create tarballs to also cover scripts/ dir #213
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
Conversation
ocaisa
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me, but would prefer if @boegel took a look to double-check
bedroge
left a comment
There was a problem hiding this 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 too, we can also merge EESSI/filesystem-layer#139 after this PR has been merged.
|
@huebner-m Why did no CI run on this PR? This should have triggered something (tarballing the init/scripts is pretty easy and indirectly checks utils.sh) |
That's because @huebner-m hasn't contributed to this repo yet, then CI doesn't run (that's done to avoid leaking secrets like tokens to "contributors" with malicious intent). |
|
The |
… into move_bash_utils
| - utils.sh | ||
| - scripts/utils.sh | ||
| - update_lmod_cache.sh | ||
| - create_directory_tarballs.sh |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This only makes sure that this workflow is triggered when changes are made to create_directory_tarballs.sh - we're not actually testing create_directory_tarballs.sh directly in this workflow... Could/should we?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test should work now (it did locally). However, the CI will still fail until this PR is merged because the script downloads a tarball of the main branch to build the final tarballs and the new subdirectory doesn't exist there yet.
boegel
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
….6-gompi-foss/2022a
{2023.06}[foss/2022a] CDO v2.0.6
…to nessi-23.06-add-R-2022a fix merge conflict after EESSI#213
This is a suggestion for a new directory to be shipped with EESSI. We could use this directory to ship the scripts needed to add NVIDIA GPU support, see e.g. #212