search

carvel-dev / carvel

Carvel provides a set of reliable, single-purpose, composable tools that aid in your application building, configuration, and deployment to Kubernetes. This repo contains information regarding the Carvel open-source community.
https://carvel.dev/
Apache License 2.0
369 stars 108 forks source link

Proposal: Make vendir lazy: don't syncing if the config has not changed #675

Closed fritzduchardt closed 9 months ago

netlify[bot] commented 10 months ago

Deploy Preview for carvel ready!

Name Link
Latest commit 6922132eaf0a618bffaf5156df0053d06b516d10
Latest deploy log https://app.netlify.com/sites/carvel/deploys/651d2132183e030008e38ca7
Deploy Preview https://deploy-preview-675--carvel.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

kumaritanushree commented 10 months ago

Looks good. First approach seems better, adding config for each contents. For an example in one vendir.yam l if there are multiple contents and only one of them require non-lazy sync then we have to sync others as well multiple times if we add flag on binary. Still wait for others opinion on this.

neil-hickey commented 10 months ago

Looks great, thank you for the proposal!

+1 to first approach being the better option.

I'm inclined to think we should include a note that the sync was not performed in vendir output. Other than that this sounds like a good option.

Semantically I am wondering is this really lazy loading? Lazy to me implies that we sync it on demand - but we are actually sync'ing on changes in the common case of this being enabled.

This actually sounds more to me like caching, we store on first load, then we just re-use the local files unless there is a remote change. I'm pondering a cache: true option -- to me it's closer to what I am expecting based on the flag name. What do others think? I can be convinced of lazy though ;). The proposal looks good to me to implement with either name.

neil-hickey commented 10 months ago

LGTM 👍

kumaritanushree commented 10 months ago

LGTM

vmunishwar commented 10 months ago

Thanks for the proposal. LGTM The first approach of adding lazy config at 'contents' level is good and vendir sync --lazy=true/false seems more readable. +1 to include a note when sync was not performed in vendir output

fritzduchardt commented 10 months ago

Your last commit was not signed do you mind signing it so that we can merge the PR when we have all the needed approvals?

If have done this just now