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

Update module dependency to latest in go.mod #21

Closed
wants to merge 2 commits into from
Closed

Update module dependency to latest in go.mod #21

wants to merge 2 commits into from

Conversation

zhi-gang-sun
Copy link
Contributor

update module dependency to latest

Changes

-:broom: Update or clean up current behavior

/kind cleanup

Fixes #20

Release Note


Docs


update module dependency to latest
@knative-prow-robot
Copy link

@zhi-gang-sun: The label(s) kind/<kind> cannot be applied, because the repository doesn't have them

In response to this:

update module dependency to latest

Changes

-:broom: Update or clean up current behavior

/kind cleanup

Fixes #20

Release Note


Docs


Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@knative-prow-robot knative-prow-robot added the kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. label Nov 27, 2020
@google-cla google-cla bot added the cla: yes Indicates the PR's author has signed the CLA. label Nov 27, 2020
@knative-prow-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: zhi-gang-sun
To complete the pull request process, please assign markusthoemmes after the PR has been reviewed.
You can assign the PR to them by writing /assign @markusthoemmes in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@knative-prow-robot
Copy link

Welcome @zhi-gang-sun! It looks like this is your first PR to knative-sandbox/kn-plugin-admin 🎉

@knative-prow-robot knative-prow-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Nov 27, 2020
@zhi-gang-sun
Copy link
Contributor Author

@zhanggbj please help to take a look at the PR when you get time, thank you.

update dependency to latest
@knative-prow-robot knative-prow-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 28, 2020
@chaozbj
Copy link
Contributor

chaozbj commented Nov 30, 2020

@zhi-gang-sun In my PR 17, I updated the go.mod same as the knative.dev/client v0.17 because I want to use autoscaler.Config.ScaleToZeroPodRetentionPeriod field which is introduced from knative/serving v0.15. I tried your go.mod but I got the below error:

☕9:28 ➜ ~/Workspace/Code/github.com/kn-plugin-admin/cmd go build -mod=mod
# knative.dev/client/pkg/serving
../../../go/pkg/mod/knative.dev/[email protected]/pkg/serving/config_changes.go:442:43: not enough arguments in call to autoscaling.ValidateAnnotations
	have (bool, map[string]string)
	want (context.Context, *autoscalerconfig.Config, map[string]string)
# knative.dev/kn-plugin-admin/pkg/command/utils
../pkg/command/utils/utils.go:27:72: not enough arguments in call to client.CoreV1().ConfigMaps(desiredCm.ObjectMeta.Namespace).Get
	have (string, "k8s.io/apimachinery/pkg/apis/meta/v1".GetOptions)
	want (context.Context, string, "k8s.io/apimachinery/pkg/apis/meta/v1".GetOptions)
../pkg/command/utils/utils.go:34:66: not enough arguments in call to client.CoreV1().ConfigMaps(desiredCm.ObjectMeta.Namespace).Update
	have (*"k8s.io/api/core/v1".ConfigMap)
	want (context.Context, *"k8s.io/api/core/v1".ConfigMap, "k8s.io/apimachinery/pkg/apis/meta/v1".UpdateOptions)
# knative.dev/kn-plugin-admin/pkg
../pkg/types.go:87:72: not enough arguments in call to params.ClientSet.CoreV1().ConfigMaps("knative-serving").Get
	have (string, "k8s.io/apimachinery/pkg/apis/meta/v1".GetOptions)
	want (context.Context, string, "k8s.io/apimachinery/pkg/apis/meta/v1".GetOptions)

I think there are some interfaces changed in new knative/client-go library and we need to change our codes if we want to adopt it, for example, in types.go, we need to fix it like:

	cm, err := params.ClientSet.CoreV1().ConfigMaps("knative-serving").Get(context.TODO(), "config-domain", metav1.GetOptions{})

This is why I updated go.mod same as the knative/client v0.17 instead of latest version.
I'm not sure if you met the same compile errors, if you have no any errors found, I want to talk with you to figure out why I can't compile successfully. :)

@zhi-gang-sun
Copy link
Contributor Author

@zhi-gang-sun In my PR 17, I updated the go.mod same as the knative.dev/client v0.17 because I want to use autoscaler.Config.ScaleToZeroPodRetentionPeriod field which is introduced from knative/serving v0.15. I tried your go.mod but I got the below error:

☕9:28 ➜ ~/Workspace/Code/github.com/kn-plugin-admin/cmd go build -mod=mod
# knative.dev/client/pkg/serving
../../../go/pkg/mod/knative.dev/[email protected]/pkg/serving/config_changes.go:442:43: not enough arguments in call to autoscaling.ValidateAnnotations
	have (bool, map[string]string)
	want (context.Context, *autoscalerconfig.Config, map[string]string)
# knative.dev/kn-plugin-admin/pkg/command/utils
../pkg/command/utils/utils.go:27:72: not enough arguments in call to client.CoreV1().ConfigMaps(desiredCm.ObjectMeta.Namespace).Get
	have (string, "k8s.io/apimachinery/pkg/apis/meta/v1".GetOptions)
	want (context.Context, string, "k8s.io/apimachinery/pkg/apis/meta/v1".GetOptions)
../pkg/command/utils/utils.go:34:66: not enough arguments in call to client.CoreV1().ConfigMaps(desiredCm.ObjectMeta.Namespace).Update
	have (*"k8s.io/api/core/v1".ConfigMap)
	want (context.Context, *"k8s.io/api/core/v1".ConfigMap, "k8s.io/apimachinery/pkg/apis/meta/v1".UpdateOptions)
# knative.dev/kn-plugin-admin/pkg
../pkg/types.go:87:72: not enough arguments in call to params.ClientSet.CoreV1().ConfigMaps("knative-serving").Get
	have (string, "k8s.io/apimachinery/pkg/apis/meta/v1".GetOptions)
	want (context.Context, string, "k8s.io/apimachinery/pkg/apis/meta/v1".GetOptions)

I think there are some interfaces changed in new knative/client-go library and we need to change our codes if we want to adopt it, for example, in types.go, we need to fix it like:

	cm, err := params.ClientSet.CoreV1().ConfigMaps("knative-serving").Get(context.TODO(), "config-domain", metav1.GetOptions{})

This is why I updated go.mod same as the knative/client v0.17 instead of latest version.
I'm not sure if you met the same compile errors, if you have no any errors found, I want to talk with you to figure out why I can't compile successfully. :)

Thank you @chaozbj let me verify it in my local and ping you.

Copy link

@zhanggbj zhanggbj 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 PR! Please make sure ./hack/build.sh runs successfully (as we don't have test-infra setup yet)

@zhanggbj
Copy link

@zhi-gang-sun looks like build failed due to new Serving api change, would you please help to take a look? Thanks

# knative.dev/client/pkg/serving
vendor/knative.dev/client/pkg/serving/config_changes.go:442:43: not enough arguments in call to autoscaling.ValidateAnnotations
	have (bool, map[string]string)
	want (context.Context, *autoscalerconfig.Config, map[string]string)
# knative.dev/kn-plugin-admin/pkg
pkg/types.go:87:72: not enough arguments in call to params.ClientSet.CoreV1().ConfigMaps("knative-serving").Get
	have (string, "k8s.io/apimachinery/pkg/apis/meta/v1".GetOptions)
	want (context.Context, string, "k8s.io/apimachinery/pkg/apis/meta/v1".GetOptions)
# knative.dev/kn-plugin-admin/pkg/command/utils
pkg/command/utils/utils.go:27:72: not enough arguments in call to client.CoreV1().ConfigMaps(desiredCm.ObjectMeta.Namespace).Get
	have (string, "k8s.io/apimachinery/pkg/apis/meta/v1".GetOptions)
	want (context.Context, string, "k8s.io/apimachinery/pkg/apis/meta/v1".GetOptions)
pkg/command/utils/utils.go:34:66: not enough arguments in call to client.CoreV1().ConfigMaps(desiredCm.ObjectMeta.Namespace).Update
	have (*"k8s.io/api/core/v1".ConfigMap)
	want (context.Context, *"k8s.io/api/core/v1".ConfigMap, "k8s.io/apimachinery/pkg/apis/meta/v1".UpdateOptions)

@zhi-gang-sun
Copy link
Contributor Author

close this PR and will submit a new one with latest changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Indicates the PR's author has signed the CLA. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bump kn-admin dependencies
4 participants