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

Clarify scope of cache (shared between tasks) #242

Merged
merged 2 commits into from
Apr 23, 2019
Merged

Clarify scope of cache (shared between tasks) #242

merged 2 commits into from
Apr 23, 2019

Conversation

dotdoom
Copy link
Contributor

@dotdoom dotdoom commented Apr 14, 2019

No description provided.

Copy link
Contributor

@fkorotkov fkorotkov left a comment

Choose a reason for hiding this comment

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

Thank you for the suggestion! It's a nice tip to have explicitly in the documentation. Suggested two nit picks. What do you think about them?

@@ -154,6 +154,10 @@ test_task:
Tasks for PRs upload caches to a separate caching namespace to not interfere with caches used by other tasks.
But such PR tasks **can read** all caches even from the main caching namespace for a repository.

!!! warning "Scope of cached artifacts"
Cache artifacts are shared between tasks, so two caches with the same name on e.g. Linux and OSX containers will share the same set of files.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Cache artifacts are shared between tasks, so two caches with the same name on e.g. Linux and OSX containers will share the same set of files.
Cache artifacts are shared between tasks, so two caches with the same name on e.g. Linux containers and macOS VMs will share the same set of files.

@@ -154,6 +154,10 @@ test_task:
Tasks for PRs upload caches to a separate caching namespace to not interfere with caches used by other tasks.
But such PR tasks **can read** all caches even from the main caching namespace for a repository.

!!! warning "Scope of cached artifacts"
Cache artifacts are shared between tasks, so two caches with the same name on e.g. Linux and OSX containers will share the same set of files.
This may introduce binary incompatibility between caches. To avoid that, add `uname -ms` into `fingerprint_script` which will distinguish caches based on OS and machine architecture.
Copy link
Contributor

Choose a reason for hiding this comment

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

Use CIRRUS_OS :

Suggested change
This may introduce binary incompatibility between caches. To avoid that, add `uname -ms` into `fingerprint_script` which will distinguish caches based on OS and machine architecture.
This may introduce binary incompatibility between caches. To avoid that, add `echo $CIRRUS_OS` into `fingerprint_script` which will distinguish caches based on OS.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! TIL about suggestions on GitHub.

Do you think CIRRUS_OS alone would be sufficient? For binary compatibility, machine type (like x86, amd64, ARM etc) may also be important.

Copy link
Contributor

Choose a reason for hiding this comment

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

At the moment I think it's enough. Cirrus supports amd64 only at the moment. 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

I don’t really see a point in having 32 bit anyway.

Copy link
Contributor

Choose a reason for hiding this comment

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

Test some IOT stuff 🤷‍♂️ There is some interest in #218

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

@fkorotkov
Copy link
Contributor

Hey @dotdoom, could you please check my suggestions? Would love to merge your changes!

@dotdoom
Copy link
Contributor Author

dotdoom commented Apr 23, 2019

@fkorotkov done! I'm struggling to understand why mdspell doesn't like my "e.g." but still likes the other one in the same file, but I'm hesitant to add it to .spelling.

@RDIL
Copy link
Contributor

RDIL commented Apr 23, 2019

It has the ignore acronyms flag, strange.

@fkorotkov fkorotkov merged commit 150e41b into cirruslabs:master Apr 23, 2019
@RDIL
Copy link
Contributor

RDIL commented Apr 23, 2019

how does one approve after merging 🤨

@dotdoom dotdoom deleted the cache-scope branch May 30, 2019 14:01
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.

3 participants