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

Add new UDC to perform an optimized git deep clone #22

Merged
merged 14 commits into from
Nov 14, 2023

Conversation

idodod
Copy link
Contributor

@idodod idodod commented Nov 3, 2023

Also:

  • Moving non language UDCs under utils (+INSTALL_DIND should be used as ./utils/dind+INSTALL).
  • Standardizing tests between ssh and git directories.
  • Running dind+INSTALL tests for arm64 in a dedicated satellite.

@CLAassistant
Copy link

CLAassistant commented Nov 3, 2023

CLA assistant check
All committers have signed the CLA.

ssh/Earthfile Outdated Show resolved Hide resolved
git/Earthfile Outdated
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@vladaionescu should I maybe move git and ssh folders into a new tools folder
(and later also DIND_INSTALL into dind)?

Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we should move all three into such a common / misc / utitlities folder. We can keep DIND_INSTALL in the root too for backwards compatibility.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep, that's what I was trying to suggest :)

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 moved everything under utils, lmk if you prefer a different name.

Copy link
Member

Choose a reason for hiding this comment

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

Looks good!

Should github.com/earthly/lib/utils expose the UDCs directly? (We can still have the implementation in subdirs)

Copy link
Member

Choose a reason for hiding this comment

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

Hmm.. I wouldn't do it. It just adds more complexity to the language, with little benefit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so we're left only with some variation of exposing the UDCs in utils/Earthfile, aren't we?

Copy link
Member

Choose a reason for hiding this comment

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

I think that's the best option

Copy link
Contributor Author

@idodod idodod Nov 8, 2023

Choose a reason for hiding this comment

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

This is the alternative

IMPORT github.com/earthly/lib/utils/git:<tag>
IMPORT github.com/earthly/lib/utils/ssh:<tag>

but this would probably mean cloning more than once, unless it's cached/deduped?

Copy link
Member

Choose a reason for hiding this comment

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

It's a reasonable alternative - especially if sticking with the naming dind+INSTALL (as opposed to utils+INSTALL_DIND). The main downside of this is having to import each UDC separately. Arguably, you might not use multiple lib UDCs for this to be a problem. Maybe it can be in the future?

Having utils split up into subdirectories helps with segregating UDCs into different areas of concern nicely. But it also has the drawback that it's slightly more confusing what tag to use for each.

All in all, I'm ok with either option. There are pros and cons for each.

@idodod idodod marked this pull request as ready for review November 3, 2023 19:40
@idodod idodod requested a review from a team as a code owner November 3, 2023 19:40
Earthfile Outdated Show resolved Hide resolved
@@ -2,56 +2,7 @@ VERSION 0.7

INSTALL_DIND:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@vladaionescu
I'm not even sure we need to keep this for backwards compatibility since we're going to cut a new release/tag for utils and discontinue the global tag (which users can still use).
Thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

I would keep it, because we have many users of INSTALL_DIND who don't use a tag at all. (We should update our docs too to suggest using a tag)

I wonder if we should add a RUN echo <some-warning-message> when using the outter INSTALL_DIND, to invite the users to use the new version directly + a tag.

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 think that'd a good idea

@idodod idodod merged commit 489a6a4 into main Nov 14, 2023
3 checks passed
@idodod idodod deleted the ido-add-git-deep-clone-udc branch November 14, 2023 06:37
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.

4 participants