knative-extensions / kn-plugin-admin

Kn plugin for managing a Kubernetes based Knative installation.
Apache License 2.0
7 stars 18 forks source link

Update module dependency to latest in go.mod #21

Closed zhi-gang-sun closed 3 years ago

zhi-gang-sun commented 3 years ago

update module dependency to latest

Changes

-:broom: Update or clean up current behavior

-

/kind cleanup

Fixes #20

Release Note

Docs

knative-prow-robot commented 3 years ago

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

In response to [this](https://github.com/knative-sandbox/kn-plugin-admin/pull/21): >update module dependency to latest > > > ># Changes > > > >-:broom: Update or clean up current behavior >- >- > > >/kind cleanup > > >Fixes #20 > > > >**Release Note** > > >```release-note > >``` > >**Docs** > > >```docs > >``` > Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
knative-prow-robot commented 3 years ago

[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: - **[OWNERS](https://github.com/knative-sandbox/kn-plugin-admin/blob/master/OWNERS)** 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 commented 3 years ago

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

zhi-gang-sun commented 3 years ago

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

chaozbj commented 3 years ago

@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/client@v0.18.4/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 commented 3 years ago

@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/client@v0.18.4/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.

zhanggbj commented 3 years ago

@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 commented 3 years ago

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