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

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

Merged
merged 3 commits into from
Oct 5, 2023

Conversation

fritzduchardt
Copy link
Contributor

No description provided.

@netlify
Copy link

netlify bot commented Aug 23, 2023

Deploy Preview for carvel ready!

Name Link
🔨 Latest commit 6922132
🔍 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.

Copy link
Member

@joaopapereira joaopapereira left a comment

Choose a reason for hiding this comment

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

Some process-related changes before we get to the approval phase.
The proposal looks good, but let us wait for the other approvers to have a look

---
title: "Lazy Synching"
authors: [ "Fritz Duchardt <[email protected]>" ]
status: "draft"
Copy link
Member

Choose a reason for hiding this comment

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

Can you please change this to "In Review"

title: "Lazy Synching"
authors: [ "Fritz Duchardt <[email protected]>" ]
status: "draft"
approvers: [ ]
Copy link
Member

Choose a reason for hiding this comment

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


To force a sync despite the `lazy` setting, a new option is added to the vendir binary, e.g.
```
vendir sync --eager
Copy link
Contributor

Choose a reason for hiding this comment

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

I would lean towards having something like --ignore-lazy to have a flag more obviously associated with the field name in vendir config.
I'd imagine this would be more helpful to non-native English speakers!

Copy link
Contributor

Choose a reason for hiding this comment

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

+1, if we are saying it as lazy configuration then --ignore-lazy makes more sense.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would lean towards having something like --ignore-lazy to have a flag more obviously associated with the field name in vendir config.

+1 to have clearer association.

i would even go a step further and following existing carvel convention to favor noun based flags. so concretely:

  • --lazy=true (default) respects lazy specified sections
  • --lazy=false disables lazy evaluation and just forces download for anything

Copy link
Member

Choose a reason for hiding this comment

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

@fritzduchardt do you mind doing the change above? I think that after that we will be ready to merge this proposal

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have done it just now

@kumaritanushree
Copy link
Contributor

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
Copy link
Contributor

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.

- path: vendor
contents:
- path: custom-repo-custom-version
hash: e8a5d1511f2eb22b160bb849e5c8be39da1c4ffa5fd56ded71ff095a8b24720b
Copy link
Contributor

Choose a reason for hiding this comment

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

let's rename hash (too generic) field to something like configDigest -- i'm thinking in future we may need to implement contentDigest for ensuring content stays exactly as expected.

@neil-hickey
Copy link
Contributor

LGTM 👍

@kumaritanushree
Copy link
Contributor

LGTM

Copy link
Member

@joaopapereira joaopapereira left a comment

Choose a reason for hiding this comment

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

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?

@vmunishwar
Copy link
Contributor

vmunishwar commented Sep 7, 2023

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
Copy link
Contributor Author

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

Signed-off-by: Fritz Duchardt <[email protected]>
Copy link
Member

@joaopapereira joaopapereira left a comment

Choose a reason for hiding this comment

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

LGTM

@joaopapereira joaopapereira merged commit 9fdfa49 into carvel-dev:develop Oct 5, 2023
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

7 participants