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

feat(git): allow to decide if we want to fetch submodules or not #1758

Merged
merged 5 commits into from
Nov 12, 2023

Conversation

mavimo
Copy link
Contributor

@mavimo mavimo commented Nov 11, 2023

Fix #1732

If will allow to define if the submodule need to be fetched or not when a repository is initialised.

Test

To test this pull request, you can run the following commands:

pushd pkg/plugins/scms
go test
popd

pushd pkg/plugins/utils/gitgeneric
go test
popd

Additional Information

Tradeoff

Potential improvement

@olblak
Copy link
Member

olblak commented Nov 11, 2023

Thanks @mavimo for opening this pullrequest.

I recently stumbled on this problem and I asked my self if it was needed and you confirm that it is.

@olblak
Copy link
Member

olblak commented Nov 11, 2023

Something worth mentioning, we always try to avoid breaking changes between Updatecli releases so something we use is the feature flag experimental like here

So if we need to introduce a breaking change, we gate it before the experimental flag for a few release while we communicate about it.

@mavimo
Copy link
Contributor Author

mavimo commented Nov 11, 2023

@olblak I was thinking more to keep the current behaviour as default (aka by default is "true"), more than including a flag for that, whyt?
At the moment the only way to use this approach is to use a nullable pointer in the yaml unmarshaling (unless we include something like https://github.com/creasty/defaults ), not sure if we want to move on this direction (I push a change in the code here)

@olblak
Copy link
Member

olblak commented Nov 12, 2023

@olblak I was thinking more to keep the current behaviour as default (aka by default is "true"), more than including a flag for that, whyt? At the moment the only way to use this approach is to use a nullable pointer in the yaml unmarshaling (unless we include something like https://github.com/creasty/defaults ), not sure if we want to move on this direction (I push a change in the code here)

Great then, I didn't think about using a pointer to keep the current behavior :)

@mavimo mavimo marked this pull request as ready for review November 12, 2023 11:34
@olblak
Copy link
Member

olblak commented Nov 12, 2023

Nice pullrequest. It's really awesome the effort put on the testing.
I'll wait for the test to pass and merge unless something goes wrong

@olblak olblak added the enhancement New feature or request label Nov 12, 2023
@olblak olblak changed the title feat: allow to decide if we want to fetch submodules or not feat(git): allow to decide if we want to fetch submodules or not Nov 12, 2023
@olblak olblak added scm-github SCM of type GiHhub scm-git SCM of kind "Git" scm-gittea scm-gitlab labels Nov 12, 2023
@olblak olblak modified the milestone: 0.66.0 Nov 12, 2023
Copy link
Member

@olblak olblak left a comment

Choose a reason for hiding this comment

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

All things appears good, thanks for the pullrequest

@olblak olblak merged commit a7ac236 into updatecli:main Nov 12, 2023
6 checks passed
@mavimo mavimo deleted the allow-to-disable-submodules branch November 12, 2023 11:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request scm-git SCM of kind "Git" scm-github SCM of type GiHhub scm-gitlab scm-gittea
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants