-
Notifications
You must be signed in to change notification settings - Fork 12
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
Conversation
git/Earthfile
Outdated
There was a problem hiding this comment.
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
)?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
@@ -2,56 +2,7 @@ VERSION 0.7 | |||
|
|||
INSTALL_DIND: |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
Also:
utils
(+INSTALL_DIND
should be used as./utils/dind+INSTALL
).dind+INSTALL
tests for arm64 in a dedicated satellite.