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

Stop importing the k8s.io/kubernetes module. #6411

Merged
merged 7 commits into from
Jul 6, 2023

Conversation

absoludity
Copy link
Contributor

Description of the change

While trying to update other dependencies, I hit the issue that we're replacing certain k8s modules to be able to import k8s.io/kubernetes (even though that's not supported) but which conflict with versions required by other updates.

This PR removes the need for us to import from k8s.io/kubernetes (which we shouldn't be doing anyway), since we were only using it for the DockerCredential struct parsing - which I've moved into our own pkg/kube module.

Benefits

Less struggles with k8s version imports (no more replaces)

Possible drawbacks

Can't see any.

Signed-off-by: Michael Nelson <minelson@vmware.com>
@netlify
Copy link

netlify bot commented Jul 5, 2023

Deploy Preview for kubeapps-dev canceled.

Name Link
🔨 Latest commit 3fa1389
🔍 Latest deploy log https://app.netlify.com/sites/kubeapps-dev/deploys/64a63fa47fb68000079400a8

Copy link
Contributor

@antgamdia antgamdia left a comment

Choose a reason for hiding this comment

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

Thanks for the changes! Finally getting rid of this replace directive is awesome!

I couldn't resist reviewing this PR and adding some legal(ish)-related comments 😇

)

// The following is Copyright 2014 The Kubernetes Authors
// and icensed under the Apache License, Version 2.0.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we, perhaps, need to explicitly use the same copyright text, I was thinking sth more like:

Also, should we add the permalink instead of the link pointing to master? Not sure :S

// Including here instead of importing from k8s.io/kubernetes/credentialprovider
// since k8s.io/kubernetes is not supported for imports and leads to version issues.

// the following pieces of code have been extracted from 
// https://github.com/kubernetes/kubernetes/blob/master/pkg/credentialprovider/provider.go
// and they are subject to the undermentioned license terms.

/*
Copyright 2014 The Kubernetes Authors.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

    http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great, will update.

@@ -0,0 +1,116 @@
// Copyright 2019-2022 the Kubeapps contributors.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// Copyright 2019-2022 the Kubeapps contributors.
// Copyright 2023 the Kubeapps contributors.

Also, but unrelated, I think we should update the NOTICE file and remove this old file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, I've been removing the old OSL license files later (I don't think we personally need to add them to the root of the repo anyway, given that we upload them to the release artifacts and leave them there.. wdyt?)

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we personally need to add them to the root of the repo

The old ones? No, I don't think so. As for the current one (the one to be released), we need it just before tagging the release, this way it would end up in the release artifact.
Right after that... I believe we can safely remove it; in fact, that file no longer represents the current status of the project, as we might add/remove deps and, moreover, it is just a document to be embedded in a release artifact.

That said... I think it's easier for us to just remove it as part of the release process: remove the old one and add the new one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right after that... I believe we can safely remove it; in fact, that file no longer represents the current status of the project, as we might add/remove deps and, moreover, it is just a document to be embedded in a release artifact.

Which is why I think it's more useful uploading it as a (separate) release artifact for that release, rather than temporarily including it in the repo. But IANAL.

@absoludity
Copy link
Contributor Author

Thanks for the changes! Finally getting rid of this replace directive is awesome!

YES!

I couldn't resist reviewing this PR and adding some legal(ish)-related comments 😇

haha - I was >< close from adding you as a reviewer yesterday just to have a second set of eyes on the change. Thanks for the input! I'll change locally now, still have to sort out CI failures.

Thanks Antonio!

Signed-off-by: Michael Nelson <minelson@vmware.com>
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

license-eye has totally checked 1637 files.

Valid Invalid Ignored Fixed
811 1 825 0
Click to see the invalid file list
  • pkg/kube/dockerconfig.go

@@ -0,0 +1,133 @@
// Copyright 2023 the Kubeapps contributors.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// Copyright 2023 the Kubeapps contributors.
// Copyright 2023 the Kubeapps contributors
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
// Copyright 2023 the Kubeapps contributors.

Signed-off-by: Michael Nelson <minelson@vmware.com>
Signed-off-by: Michael Nelson <minelson@vmware.com>
Signed-off-by: Michael Nelson <minelson@vmware.com>
Signed-off-by: Michael Nelson <minelson@vmware.com>
@absoludity absoludity merged commit 3553156 into main Jul 6, 2023
@absoludity absoludity deleted the remove-kubernetes-import branch July 6, 2023 04:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants