Skip to content

Conversation

@NarmalaSk
Copy link

@NarmalaSk NarmalaSk commented Jan 31, 2025

Pull Request check-list

  • Do tests and lints pass with this change?
  • Do the CI tests pass with this change (enable it first in your forked repo and wait for the github action build to finish)?
  • Is the new or changed code fully tested?
  • Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?
  • Is there an example added to the examples folder (if applicable)?

Description of change

@NarmalaSk NarmalaSk changed the title Update integration.yaml Update integration.yaml Dynamic Caching Docker in ci with cron Jan 31, 2025
@mkmkme
Copy link
Collaborator

mkmkme commented Feb 3, 2025

Hey! Thanks for your contribution. Do I understand correctly that this PR targets #140 ?

@NarmalaSk
Copy link
Author

Hey! Thanks for your contribution. Do I understand correctly that this PR targets #140 ?

Yeah it resolves the issue ,previously Docker cache runs at midnight but it would'nt update it now I have added a timeout now it updates docker cahe

@mkmkme
Copy link
Collaborator

mkmkme commented Feb 3, 2025

Thanks! Your changes are quite large because you have your yaml formatter enabled on IDE. Could you disable it and force-push this branch so I could have a review of the intended changes?

Also for me it seems like this change makes it so the docker cache rebuilds every run. Which might be fine, but originally I was thinking about doing it daily. The original idea of docker caching was done by @smeso, so I'll ask him what he had in mind :)

This reverts commit b8698a9.

Please enter the commit message for your changes. Lines starting
 with '#' will be ignored, and an empty message aborts the commit.
 On branch main
 Your branch is up to date with 'origin/main'.

 Changes to be committed:
@mkmkme
Copy link
Collaborator

mkmkme commented Feb 4, 2025

Hey @NarmalaSk ! Is it expected that the PR is now empty? Or did you forget to push the changes?

@NarmalaSk
Copy link
Author

Hey @NarmalaSk ! Is it expected that the PR is now empty? Or did you forget to push the changes?

Yes Pr is empty now , I think my changes would change whole yaml file docker cache process , that's why Haven't pushed the changes

@mkmkme
Copy link
Collaborator

mkmkme commented Feb 4, 2025

Changing the caching process should be fine I guess, as long as the commit doesn't include the changes outside the scope of the problem :)

Please tell me if you still are interested in fixing this issue. If you are not, I'll close this PR.

@NarmalaSk NarmalaSk changed the title Update integration.yaml Dynamic Caching Docker in ci with cron Update integration.yaml Feb 4, 2025
@NarmalaSk
Copy link
Author

Changing the caching process should be fine I guess, as long as the commit doesn't include the changes outside the scope of the problem :)

Please tell me if you still are interested in fixing this issue. If you are not, I'll close this PR.

now i have changed the integrations file you can look up to the code if it is ok merge else close this pr

@mkmkme
Copy link
Collaborator

mkmkme commented Feb 4, 2025

Sorry but it still includes many changes related to your lint. Additionally, you changed the title to be less descriptive of the changes themselves.

I would like to thank you for your idea regarding caching! I'll have a look at the ways to implement it in our codebase as soon as I have time for it. If you struggle with your formatter, it's fine -- I can create a new PR later with only the change related to the docker cache. If this way of handling docker cache is relevant to our case, I'll mark you as the co-author of the changes :)
However, if you manage your code formatter, feel free to force-push the commit that includes only the changes related to the docker cache to this PR!

@mkmkme mkmkme mentioned this pull request Feb 26, 2025
5 tasks
@mkmkme
Copy link
Collaborator

mkmkme commented Feb 26, 2025

Thanks for your PR! I've decided to tweak your approach a bit and create a cache every day instead of creating it for every commit. I'd say creating it every day makes more sense in our use case because we cache Valkey docker images, and they don't get updated that often.

I will now close this PR.

@mkmkme mkmkme closed this Feb 26, 2025
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