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

feat: Adding 'save-cache' property for S3 cache #57

Merged
merged 4 commits into from
Mar 31, 2023

Conversation

ingcsmoreno
Copy link

@ingcsmoreno ingcsmoreno commented Mar 29, 2023

What it does?

It makes the plugin saves the tar file on /tmp after downloading it for the first time, and re-uses that file on any further execution.

This is to complement the #36 implementation in further executions of builds on the same node, and when the same cache file is used by multiple pipelines.

How can I test it?

I've created a test to check it works.

Also, use this Plugin from this Fork on a pipeline, and enable the property like this:

...

  - plugins:
    - gencer/cache#v2.4.13:
        id: ruby # or ruby-3.0
        backend: s3
        key: "v1-cache-{{ id }}-{{ runner.os }}-{{ checksum 'Gemfile.lock' }}"
        restore-keys:
          - 'v1-cache-{{ id }}-{{ runner.os }}-'
          - 'v1-cache-{{ id }}-'
        s3:
          prefere-local: true
        paths:
          - 'bundle/vendor'
...

Issues

Extra Comments

This is not really to fully address #36 but to complement it.

@gencer
Copy link
Collaborator

gencer commented Mar 29, 2023

Hey @ingcsmoreno!

Thanks for the PR. I see the point and I think this is a great feature, but I'm bit unsure about naming: prefere-local. Should it be prefer-local or something else? If you have any other suggestion or word about this, let me know, Otherwise I'll merge the PR today late-evening.


As a bonus, thanks for the version bump & release note addition too. A great favor for me. I usually await for few features until a bump but nowadays I just release even single features too.

@ingcsmoreno
Copy link
Author

Hey @ingcsmoreno!

Thanks for the PR. I see the point and I think this is a great feature, but I'm bit unsure about naming: prefere-local. Should it be prefer-local or something else? If you have any other suggestion or word about this, let me know, Otherwise I'll merge the PR today late-evening.

As a bonus, thanks for the version bump & release note addition too. A great favor for me. I usually await for few features until a bump but nowadays I just release even single features too.

Thanks! I agree the property name could be more descriptive, but I don't want it to be too long either. Maybe something like add-local-cache, local-cache-enabled, prevent-download-if-local? I'm open to suggestions :D

@gencer
Copy link
Collaborator

gencer commented Mar 29, 2023

...

  - plugins:
    - gencer/cache#v2.4.13:
        id: ruby # or ruby-3.0
        backend: s3
        key: "v1-cache-{{ id }}-{{ runner.os }}-{{ checksum 'Gemfile.lock' }}"
        restore-keys:
          - 'v1-cache-{{ id }}-{{ runner.os }}-'
          - 'v1-cache-{{ id }}-'
        s3:
          cache: true # keep the cache between builds/jobs on the same machine
        paths:
          - 'bundle/vendor'
...

I think only one word cache will work much better than 2-3 words with dashes... Or if we want more descriptive key:

keep-cache: true # keep the cache between builds/jobs on the same machine
# or
save-cache: true # save the cache on temp folder and keep between builds/jobs on the same machine

As soon as we put this information on README it doesn't matter how short or long the key is.

@gencer gencer self-assigned this Mar 29, 2023
@ingcsmoreno ingcsmoreno changed the title feat: Adding 'prefere local' property for S3 cache feat: Adding 'save-cache' property for S3 cache Mar 29, 2023
@ingcsmoreno
Copy link
Author

@gencer save-cache sounds good, I've just updated the PR and files. Thanks!

@gencer
Copy link
Collaborator

gencer commented Mar 30, 2023

@ingcsmoreno As you might notice, CI fails on this PR.

@ingcsmoreno
Copy link
Author

@gencer looks like it's complaining about the s3 -> profile yaml syntax?

I executed the lint on master and it also fails with the same issue. Doesn't seem to be related with my changes. IMHO seems to be something related with the linter image code.

Could you give me a hand to figure this out?

@ingcsmoreno
Copy link
Author

@gencer The CICD check is fixed!

@gencer gencer merged commit a0c7a39 into nienbo:master Mar 31, 2023
@gencer
Copy link
Collaborator

gencer commented Apr 1, 2023

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.

2 participants