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

Potential import collision: import path should be "k8s.io/helm", not "github.com/helm/helm". #7

Closed
KateGo520 opened this issue Aug 11, 2020 · 5 comments
Assignees

Comments

@KateGo520
Copy link

Background

I find that k8s.io/helm and github.com/helm/helm coexist in this repo:
https://github.com/xchapter7x/hcunit/blob/master/go.mod (Line 15 & 31)

github.com/helm/helm v2.14.3+incompatible 
k8s.io/helm v2.14.3+incompatible 

The helm/helm has already renamed it’s import path from "github.com/helm/helm" to "k8s.io/helm".
https://github.com/helm/helm/blob/v2.14.3/pkg/chartutil/values_test.go#L31

package chartutil
import (
	…
	"k8s.io/helm/pkg/proto/hapi/chart"
	"k8s.io/helm/pkg/timeconv"
	"k8s.io/helm/pkg/version"
)
…

The "k8s.io/helm" and "github.com/helm/helm" are the same repos. This will work in isolation, bring about potential risks and problems.

So, why not get rid of the old import path "github.com/helm/helm", use "k8s.io/helm" instead.

Solution

Replace all the old import paths, change "github.com/helm/helm" to "k8s.io/helm".
Where did you import it: https://github.com/xchapter7x/hcunit/search?q=github.com%2Fhelm%2Fhelm&unscoped_q=github.com%2Fhelm%2Fhelm

@KateGo520
Copy link
Author

@xchapter7x Could you help me review this issue? Thx :p

@github-actions
Copy link

Welcome to the community and thank you for adding this issue :)' first issue

@xchapter7x
Copy link
Owner

Hi @KateGo520

Thanks for bringing this up.
Honestly, I think this was just an oversight and an error on my part.

I'd be happy to merge in a PR if you have a WIP in a branch already?
If not, I can make the suggested changes.

One question I have though, is this causing an issue somewhere? Is there a breaking test case I can add somewhere or is this just poor dependency hygiene?

Thanks for being engaged. I hope you are finding value in hcunit. Please let me know if you have any suggestions or feedback on this tool. Insights on how you are using it would be most helpful as well :)

@xchapter7x xchapter7x self-assigned this Aug 13, 2020
xchapter7x added a commit that referenced this issue Aug 13, 2020
we should only use a single version of a dependency.
this fixes an issue where we were importing helm libs
from 2 different sources of truth.

#7
@xchapter7x
Copy link
Owner

this use of duplicate replica dependencies has now been addressed: 
3fdf4a5

and is in the latest release:
https://github.com/xchapter7x/hcunit/releases/tag/v0.7.5

@KateGo520
Copy link
Author

Thanks for your efforts and feedback. @xchapter7x

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants