hg: Add a cache for mercurial repositories.

[cdevienne]

hg: Add a cache for mercurial repositories.

The idea is to save the whole untouched clone (with no checkout) in the cache.

If already present, the fetch (a pull actually for hg) is done directly in the cache, and is
faster (except on very small repos) because only new changesets are
transfered.

If the ref is a changeset id (not a tag, branch, topic or bookmark), and
the changeset is already known in the cached clone, no pull is done
which avoid any network exchange.

Then we copy the cached entry and do the checkout.

The same feature for git will be proposed very soon, so any feedback on this one is welcome if it helps doing things better for git!

[kumaritanushree]

@cdevienne not sure how much effort will require but if possible can you please add test for this?

[cdevienne]

@kumaritanushree I will write some tests. Do you need more precisions on the CacheID logic I implemented?

[joaopapereira]

Added some questions and left some comments.

[joaopapereira]

Do you mind fixing the rebase so that we do not have all the other changes in this PR?

[cdevienne]

I un-exported the "Hg" type completely, as it has no reasonable use outside this package.

[Zebradil]

@joaopapereira should I be able to approve workflow runs?

[praveenrewar]

@Zebradil Are you not able to approve? Let me check the permissions in that case. For now I am approving.

[Zebradil]

@praveenrewar No, I didn't have the button.

[Zebradil]

@cdevienne thank you for the adjustments. Now we can see that the localClone function is unused. Could you remove it? Or was it supposed to be used at some point?

[praveenrewar]

No, I didn't have the button.

@Zebradil Please check now. I am still figuring out some of the permissions stuff, so please excuse me if it still doesn't work 😅

[Zebradil]

I can't test if it works now, as there are no pipelines to approve :)
Let's wait until we have another PR.

[cdevienne]

@cdevienne thank you for the adjustments. Now we can see that the localClone function is unused. Could you remove it? Or was it supposed to be used at some point?

my bad I forgot to run golangci-lint. I cleaned up the code.

[Zebradil]

@cdevienne sorry, I didn't notice earlier that with the new functions some existing structs (Hg, HgInfo, ...) and functions (NewHg, ...) became unexported too. This could be a breaking change for someone who might use them in their code. Could you revert this change to the said structs and functions?

[Zebradil]

@praveenrewar I don't see the button to approve workflow runs.

[cdevienne]

@cdevienne sorry, I didn't notice earlier that with the new functions some existing structs (Hg, HgInfo, ...) and functions (NewHg, ...) became unexported too. This could be a breaking change for someone who might use them in their code. Could you revert this change to the said structs and functions?

It is a breaking change indeed, but I would be surprised it breaks anything. I don't see a point in keeping these public if it doesn't break any code. If it does I would happily revert the change.

[cdevienne]

Precising my point: I think the internal of the fetchers are not a part of the contract vendir has with its users. So it should not have been public in the first place.

[Zebradil]

I think the internal of the fetchers are not a part of the contract vendir has with its users.

I doubt that this code is used outside of vendir. However, it is located under pkg which, if I understand correctly, makes it "public". If it were under internal I wouldn't bother much.

I don't see a point in keeping these public if it doesn't break any code.

Yes, but how do we know if it doesn't break someone else's code?

@joaopapereira could you give a suggestion here?

Removing symbols from public API is a breaking change according to the semantic versioning spec and we should implement it carefully with adequate communication. But vendir's major version is still 0 which gives us some freedom to break the public API. I personally would commit this (breaking) change and see who reacts on this and what are their use cases. Also, ytt, for example, discourages using it as a module: https://github.com/carvel-dev/ytt/blob/develop/examples/integrating-with-ytt/apis.md#as-a-go-module. I think for vendir these considerations make sense too.

[joaopapereira]

As a generic rule of thumb in Carvel we tend to:

• Minimize the breaking changes, our tools are still all in the major 0 version, which gives us some leeway to do breaking changes. Nevertheless, we should aim to minimize it
• We try to keep consistency whenever possible

I like the idea of exposing only the sync, but we should standardize what this sync interface should have before we do that. This being said, I would recommend we keep exported Structs exported, and in the future, if we want to invest time in getting an interface going that would support all the fetching types, we can create the interface and make all things private that we want to make private.

In terms of API consumption for the majority of the tools, we expect people to use the Cobra commands interface (not the best experience) until we are in a position where we have a supported API for each tool like we currently have in imgpkg.

[joaopapereira]

@praveenrewar I don't see the button to approve workflow runs.

Only folks with write access can see that button, and write access comes with the Approver Role.

[cdevienne]

Should I make the structs public again ? What about their functions ?
Or can it be merged as it is ?

[joaopapereira]

Let us make them public again and we will think in the future on a way to make a good interface API for this

[cdevienne]

ok doing it. Be aware that the 'Retrieve' function does not exist anymore, and that the Hg struct has no exposed function aside from a constructor and a Close function.

[cdevienne]

Just to be clear: the Hg public interface is changing, an the new one is useless as it is. Which is why I unexposed it completely.
Pushing the change you asked for anyway, let me know if you need any other change.

[cdevienne]

Hi @joaopapereira, any chance we merge this PR soon ?

[joaopapereira]

LGTM

[joaopapereira]

@Zebradil can you give it a last review please and if you are ok with it I will merge the PR

[Zebradil]

LGTM

[cdevienne]

Thanks!