[kn-admin]Added utils to update configmap

knative / client-contrib

Community contributed `kn` plugins.

Apache License 2.0

10 stars 23 forks source link

[kn-admin]Added utils to update configmap #40

Closed zhanggbj closed 4 years ago

zhanggbj commented 4 years ago

Added utils to update configmp, as this is very common for kn-admin operations
Chores to refine vars or logs

zhanggbj commented 4 years ago

@rhuss @maximilien The integration test failed due to incompatible packages, looks like it is not one of kn-admin, woudl you please help to take a look?

# k8s.io/client-go/rest
../../../pkg/mod/k8s.io/client-go@v11.0.1-0.20190805182717-6502b5e7b1b5+incompatible/rest/request.go:598:31: not enough arguments in call to watch.NewStreamWatcher
    have (*versioned.Decoder)
    want (watch.Decoder, watch.Reporter)
ERROR: cluster setup failed

rhuss commented 4 years ago

Yep, I will give a look later today (have some meetings soon)

zhanggbj commented 4 years ago

@maximilien I addressed your comments, and would you please help to take a look? Thanks!

e2e test case not ready yet, covered by #14
Kpow integration tests failures due to repo settings (Roland is taking care)

zhanggbj commented 4 years ago

@rhuss Thanks! As the example here, kn-admin will also add the domains with/without the selector. When users need to remove a domain, they only need to provide the domain name. And kn admin domain list is working in progress, to list all the added domains and with selectors if any.

zhanggbj commented 4 years ago

@rhuss As discussed in https://github.com/knative/client-contrib/pull/40#discussion_r442624507, I opened another chore item to follow command wording create/apply/update (#53).

@rhuss @maximilien Please help to review again and hope we can merge this first before the vendor pkg change. thanks!

rhuss commented 4 years ago

/hold

rhuss commented 4 years ago

@maximilien please unhold if this looks ok for you, so that it gets merged.

knative-prow-robot commented 4 years ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: maximilien, rhuss, zhanggbj

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

The pull request process is described here

Needs approval from an approver in each of these files: - ~~[plugins/admin/OWNERS](https://github.com/knative/client-contrib/blob/master/plugins/admin/OWNERS)~~ [maximilien,rhuss,zhanggbj] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment

zhanggbj commented 4 years ago

@maximilien Thanks Max. For test, we have UT for each command and also for registry/add e2e test is still working in progress by #14

maximilien commented 4 years ago

@maximilien Thanks Max. For test, we have UT for each command and also for registry/add e2e test is still working in progress by #14

OK cool. Let me know when you address the last comments from @navidshaikh and we can merge this. Thx.

rhuss commented 4 years ago

/unhold