Skip to content
This repository has been archived by the owner on Nov 1, 2022. It is now read-only.

remove replace directives #97

Closed
wants to merge 1 commit into from

Conversation

carnott-snap
Copy link
Contributor

go.mod contains many replace entries that all consumers must include to
import logic. These are cancerous and tend to make consumers builds
difficult or outright fail. All have been removed, except for the one
required by flux. This entry, while retained, has converted into a noop
and should be removed when fluxcd/flux#2590 is fixed.

carnott-snap added a commit to carnott-snap/flux that referenced this pull request Nov 7, 2019
go.mod contains many replace entries that all consumers must include to
import logic. These are cancerous and tend to make consumers builds
difficult or outright fail. All have been removed, except for the one
required by helm-operator. This entry, while retained, has been
converted into a noop and should be removed when
fluxcd/helm-operator#97 is fixed.
@carnott-snap carnott-snap force-pushed the no-replace branch 5 times, most recently from d9c1d88 to c3a879c Compare November 8, 2019 01:57
go.mod Outdated
Comment on lines 8 to 15
// TODO(carnot-snap): force downgrade because of a trasitive upgrade
//
// github.com/fluxcd/helm-operator
// +-> github.com/fluxcd/flux@v1.15.0
// +-> github.com/fluxcd/helm-operator@v1.0.0-rc1
// +-> github.com/weaveworks/flux@v0.0.0-20190729133003-c78ccd3706b5
// +-> k8s.io/code-generator@v0.0.0-20190511023357-639c964206c2
//
// This replace should be removed once fluxcd/flux is released.
replace k8s.io/code-generator => k8s.io/code-generator v0.0.0-20190311093542-50b561225d70
Copy link
Contributor Author

Choose a reason for hiding this comment

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

removal of this dependency cycle will require synchronised commits in both helm-operator and flux:

  1. remove all require and replace blocks from helm-operator: carnott-snap/helm-operator@100d4a6
  2. pin flux to pseudo version of helm-operator: carnott-snap/flux@65da8ef
  3. remove unnecessary replace blocks:

@carnott-snap
Copy link
Contributor Author

carnott-snap commented Nov 8, 2019

Thanks, but this belongs to a separate PR.

When you say this, what would you like to see teased apart?

I think you are referring to the migration from github.com/weaveworks/flux/git to github.com/fluxcd/flux/pkg/git. We can certainly do that first, but I think it is important to make that a prerequisite to the removal of the replace directives, since that commit is the root of the kubernetes issues.

Also, I am not sure we should rewrite the history of the Changelogs, at the time of the release those tickets/PRs lived in the weaveworks repo (and the links are not broken, since GitHub redirects automatically).

My find -exec sed may have been more aggressive than you would like, but I think that it serves to stabilise the links, since at a later date weaveworks could create a flux repo and break that redirect. Github does not guarantee the redirect after a migration, just offers it when it is possible.

@2opremio
Copy link
Contributor

2opremio commented Nov 8, 2019

Yes, I am referring to the renames. They don't belong to this PR. I am still unsure of whether we should merge them but, if anything, they should be placed in a separate PR.

@carnott-snap
Copy link
Contributor Author

Yes, I am referring to the renames. They don't belong to this PR. I am still unsure of whether we should merge them but, if anything, they should be placed in a separate PR.

I have isolated and reduced that change in #101, and will wait till that merges before we proceed.

Are you interested in restarting the dependency cycle as described above, so that we can cleanup the requires? Based on the work in fluxcd/flux#2590, there should be a way to maintain the go.sum without the entries.

If we cannot come to an understanding on removing them, I would be interested to fixup the go.mod, and dependency graph, so that no requires are needed for customers.

@hiddeco
Copy link
Member

hiddeco commented Nov 12, 2019

@carnott-snap as you may have noticed I am refactoring large parts (if not all) of the code this touches in #99. Would you be open to make this PR target the helm-v3-dev branch to make the future merge of master with the ongoing development branch for support of Helm v3 easier?

It is not likely that there will be a release anyway before those two have been merged together, so release wise it does not make a difference.

@carnott-snap carnott-snap changed the base branch from master to helm-v3-dev November 12, 2019 23:58
@carnott-snap carnott-snap changed the base branch from helm-v3-dev to master November 13, 2019 00:43
@carnott-snap
Copy link
Contributor Author

@carnott-snap as you may have noticed I am refactoring large parts (if not all) of the code this touches in #99. Would you be open to make this PR target the helm-v3-dev branch to make the future merge of master with the ongoing development branch for support of Helm v3 easier?

I am happy to make the change against helm-v3-dev, however the inclusion of k8s.io/kubernetes@v1.15.4 is going to complicate things substantially. As such, I will make a separate pr to track that branch, and we should be able to land both.

go.mod contains many replace entries that all consumers must include to
import logic. These are cancerous and tend to make consumers builds
difficult or outright fail. All unnecessary replaces have been removed.

One is required by flux and should be removed when
distribution/distribution#2905 is released.

The second is caused by a co-dependant transitive upgrade that bumps
k8s.io/code-generator beyond kubernetes-1.14.4. To correct this the
go.mod has been left untidy, with the correct version required and
replaced so that build/test will pass.
@carnott-snap
Copy link
Contributor Author

carnott-snap commented Nov 13, 2019

@hiddeco, after further review, it appears that helm-operator will be unable to consume the helm.sh/helm/v3 module beyond v3.0.0-beta.3. I think we should push forward with this change and review helm-v3-dev when it has been updated to upstream@latest.

This issue is caused by your replace block downgrading k8s.io/kubectl and k8s.io/apiextensions-apiserver to a version below what helm.sh/helm/v3 needs:

github.com/fluxcd/helm-operator/pkg/helm/v3 imports
        helm.sh/helm/v3/pkg/chartutil imports
        k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1: module k8s.io/apiextensions-apiserver@latest found (v0.0.0-20191109110701-3fdecfd8e730, replaced by k8s.io/apiextensions-apiserver@v0.0.0-20190918201827-3de75813f604), but does not contain package k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1
github.com/fluxcd/helm-operator/pkg/helm/v3 imports
        helm.sh/helm/v3/pkg/kube imports
        k8s.io/kubectl/pkg/cmd/util: module k8s.io/kubectl@latest found (v0.0.0-20191111223430-169c48cabbe1, replaced by k8s.io/kubectl@v0.0.0-20190602132728-7075c07e78bf), but does not contain package k8s.io/kubectl/pkg/cmd/util
github.com/fluxcd/helm-operator/pkg/helm/v3 imports
        helm.sh/helm/v3/pkg/kube imports
        k8s.io/kubectl/pkg/validation: module k8s.io/kubectl@latest found (v0.0.0-20191111223430-169c48cabbe1, replaced by k8s.io/kubectl@v0.0.0-20190602132728-7075c07e78bf), but does not contain package k8s.io/kubectl/pkg/validation
github.com/fluxcd/helm-operator/pkg/helm/v3 imports
        helm.sh/helm/v3/pkg/kube tested by
        helm.sh/helm/v3/pkg/kube.test imports
        k8s.io/kubectl/pkg/cmd/testing: module k8s.io/kubectl@latest found (v0.0.0-20191111223430-169c48cabbe1, replaced by k8s.io/kubectl@v0.0.0-20190602132728-7075c07e78bf), but does not contain package k8s.io/kubectl/pkg/cmd/testing

@hiddeco
Copy link
Member

hiddeco commented Nov 22, 2019

@carnott-snap can you revise this PR so that it is mergeable with master, and write down the steps that you have in mind that should happen after this has been merged? As it is hard for me to determine what has and has not been done at this point. Thanks 🥇

@2opremio 2opremio removed their assignment Dec 16, 2019
@hiddeco hiddeco closed this Mar 2, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants