Skip to content

Conversation

alexandear
Copy link
Contributor

This PR introduces the giturl package with a single Parse function to parse Git URLs. No need to import the whole package just for the one function.

The giturl package is a refactored copy of https://github.com/chainguard-dev/git-urls with unexported methods and extended tests.

This PR follows the Go proverb "A little copying is better than a little dependency".

Relates to #1917

@alexandear alexandear changed the title use giturl pkg instead of forked chainguard-dev/git-urls refactor: use giturl pkg instead of chainguard-dev/git-urls Nov 12, 2024
@andreynering
Copy link
Member

Hi @alexandear,

I understand the principle of reducing dependencies, and I generally agree with it, but it is a case-by-case decision.

In this case, we're adding 500+ lines of code (including tests) to the codebase to avoid a dependency. I'm not so sure it is worth it. In this case, it might be better to just keep the dependency, specially considering it does not import other packages, so we're talking about a single smallish package here.

@pd93
Copy link
Member

pd93 commented Nov 12, 2024

@andreynering Agreed. There is nothing wrong with the dependency IMO.

@alexandear
Copy link
Contributor Author

Thank you for sharing your perspective. I understand your concerns about introducing 115 lines of code just to eliminate a single dependency. It's indeed important to weigh the benefits and potential maintenance overhead.

Removing dependencies can bring long-term benefits, such as reducing the risk of breaking changes and improving security because we fully control the package.

I've also included some articles that could provide additional insight on when it’s appropriate to minimize dependencies and when it might be acceptable to keep them:

It's ultimately up to you whether to accept this PR or not. Close the PR if it's not needed.

@pd93
Copy link
Member

pd93 commented Nov 13, 2024

Thanks for the PR. IMO most of the benefits of the principle you describe do not apply here so I'm going to close this.

An additional note. If we were to include a copy of this code in Task, it would be required that we extend our licence to cover the one in used in https://github.com/chainguard-dev/git-urls. We cannot simply take the code without crediting the author as per the MIT license.

@pd93 pd93 closed this Nov 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants