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

Use getopts in bin/save_cache #54

Draft
wants to merge 8 commits into
base: trunk
Choose a base branch
from
Draft

Conversation

mokagio
Copy link
Contributor

@mokagio mokagio commented May 12, 2023

Following up on this suggestion before looking into using the command for the Gutenberg work I'm doing at the moment.

⚠️ Apologies for the tab vs spaces changes. I'll try to fix them before merging.

WIP / RFC

I removed the CACHE_KEY computation logic if the parameter was not passed because it was never used. What do you think? I don't think this change would require a major version bump because:

We can say with confidence that it never run because 1) all the callers in this plugin pass a cache and 2) because of the u flag in the shebang, the line CACHE_KEY=$2 would have failed the script if no value had been passed, therefore never running the fallback logic. Moreover, the script uses $3 and $4 later on, further proving that $2 was always, implicitly, required.

So, while technically making $2 required when it previously wasn't should count as a breaking change, in practice $2 was always required and I think we shouldn't worry with that.

However... Since we are taking the time to add options it might be beneficial to use them for both the path to archive and the key to use. That's would make the command more robust, as it would be order agnostic. save_cache --path PATH_TO_CACHE --key KEY works just as well as save_cache --key KEY --path PATH_TO_CACHE but save_cache PATH_TO_CACHE KEY and save_cache KEY PATH_TO_CACHE produce different results. If we were to do that, then it would definitely count as a breaking change.

What do you think?


  • I have considered if this change warrants release notes and have added them to the appropriate section in the CHANGELOG.md if necessary.

mokagio added 7 commits May 12, 2023 14:10
It's not guaranteed to be a file, it can be a directory, too.
And yes, technically Unix directories are files, but the naming seemed
confusing specially when used in the `tar` command.
We can say with confidence that it never run because 1) all the callers
in this plugin pass a cache and 2) because of the `u` flag in the
shebang, the line `CACHE_KEY=$2` would have failed the script if no
value had been passed, therefore never running the fallback logic.
Moreover, the script uses `$3` and `$4` later on, further proving that
`$2` was always, implicitly, required.
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.

1 participant