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

fix: cache key stability #142

Merged
merged 2 commits into from May 18, 2023
Merged

Conversation

stevenh
Copy link
Contributor

@stevenh stevenh commented May 16, 2023

Ensure consistency of main and post configuration by storing and restoring it from state, which in turn ensures cache key stability.

Also:

  • Fixed some typos.
  • Use core.error for logging errors.
  • Fix inverted condition on cache-all-crates.

Reverts: #138
Fixes #140

@stevenh stevenh force-pushed the fix/all-creates-packages branch 4 times, most recently from 5fa5161 to 9b4b3d7 Compare May 16, 2023 23:57
@stevenh stevenh marked this pull request as ready for review May 17, 2023 00:05
@stevenh stevenh force-pushed the fix/all-creates-packages branch 2 times, most recently from 1d5cfc6 to d5a9bd4 Compare May 17, 2023 10:09
Ensure consistency of main and post configuration by storing and
restoring it from state, which in turn ensures cache key stability.

Also:
* Fixed some typos.
* Use core.error for logging errors.
* Fix inverted condition on cache-all-crates.

Reverts: Swatinem#138
Fixes Swatinem#140
Copy link
Owner

@Swatinem Swatinem left a comment

Choose a reason for hiding this comment

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

lgtm, except the Object.assign/JSON.parse roundtrips. We should make it explicit what we are serializing, and deserialize things manually as well.

src/config.ts Outdated Show resolved Hide resolved
src/config.ts Show resolved Hide resolved
Make the loading of config from state explicit by splitting into a
dedicated static method.
@stevenh
Copy link
Contributor Author

stevenh commented May 18, 2023

@Swatinem to make sure you didn't miss the notification 😃

Copy link
Owner

@Swatinem Swatinem left a comment

Choose a reason for hiding this comment

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

lgtm, though the specifics of de/serialization can be improved later

@Swatinem Swatinem merged commit ad97570 into Swatinem:master May 18, 2023
20 checks passed
@stevenh stevenh deleted the fix/all-creates-packages branch May 18, 2023 20:54
@stevenh
Copy link
Contributor Author

stevenh commented May 18, 2023

Thanks, appreciate your time.

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.

cache key is unstable with additional cargo installs
2 participants