get a particular service and revision based on name parameter by savitaashture · Pull Request #150 · knative/client

savitaashture · 2019-05-30T11:34:24Z

current:

kn service list -n test autoscale-go
NAME           DOMAIN                          GENERATION   AGE    CONDITIONS   READY     REASON
autoscale-go   autoscale-go.test.example.com   1            125m   0 OK / 3     Unknown   RevisionMissing : Configuration "autoscale-go" is waiting for a Revision to become ready.
test1          test1.test.example.com          1            119m   0 OK / 3     False     RevisionMissing : Configuration "test1" does not have any ready Revision.

Expected: Should list only that perticular service.
something like below

kn service list -n test autoscale-go
NAME           DOMAIN                          GENERATION   AGE    CONDITIONS   READY     REASON
autoscale-go   autoscale-go.test.example.com   1            125m   0 OK / 3     Unknown   RevisionMissing : Configuration "autoscale-go" is waiting for a Revision to become ready.

knative-prow-robot · 2019-05-30T11:34:39Z

Hi @savitaashture. Thanks for your PR.

I'm waiting for a knative member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

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.

rhuss · 2019-05-30T14:58:29Z

+			return nil, nil
+		}
+		revision.GetObjectKind().SetGroupVersionKind(schema.GroupVersionKind{
+			Group:   "knative.dev",


The coordinates are wrong (whould be serving.knative.dev for the Group), but we should get #134 merged as soon as possible which avoids this kind of hardcoding of the GKV. @sixolet @cppforlife can we please get #134 merged soon to avoid piling up on this technical debt ?)

Sure...Once that is merged i will rechange this

rhuss

Thanks a lot ! I have some comments though.

On a more general level: I see a lot of duplicated code. Would it make sense to unify "revision get" and "service get" where possible ?

rhuss · 2019-05-30T15:28:50Z

+
+func createMockRevisionWithParams(name, svcName string, version bool) *v1alpha1.Revision {
+	var apiVersion string
+	if version {


Why do you have different api versions ?

This condition i have added to check the apiVersion
Because in the base code
when we do

kn revision get -o yaml apiVersion: knative.dev/v1alpha1 items: - apiVersion: serving.knative.dev/v1alpha1 kind: Revision metadata: annotations: autoscaling.knative.dev/target: "10" serving.knative.dev/lastPinned: "1559284138" creationTimestamp: "2019-05-31T06:28:47Z" generateName: autoscale-go- generation: 1

If its a list then it is giving apiversion as knative.dev

and after adding my logic to get the single revision the apiversion should be serving.knative.dev
So in order to use the same function in test case i just added a boolean variable to check whether thats a get all revisions or get a revision based on name.

But as you said once after this merge
#134
GVK will be handled i think that time this condition is not required.

savitaashture · 2019-05-31T08:03:43Z

it make sense to unify

Found fakeRevisionGet and fakeServiceGet duplicate code modified and updated.

rhuss · 2019-06-06T16:18:21Z

@savitaashture thanks a lot for your updates ! Could you please rebase on latest mastrer, there has been some structural changes. Nothing big, probbaly just import conflicts.

savitaashture · 2019-06-10T09:56:07Z

@savitaashture thanks a lot for your updates ! Could you please rebase on latest mastrer, there has been some structural changes. Nothing big, probbaly just import conflicts.

@rhuss I have rebased with latest master changes.
Thank you.

rhuss · 2019-06-10T13:03:18Z

/ok-to-test

navidshaikh · 2019-06-14T11:18:01Z

@savitaashture : get is now renamed to list, please rebase and update the PR.

savitaashture · 2019-06-14T19:08:48Z

@savitaashture : get is not renamed to list, please rebase and update the PR.

@navidshaikh @rhuss Rebased

navidshaikh

Filtering a service by its name makes sense as the name is user provided, but the revision name is generated at server, how can we expect the user to remember the complicated name generated for revision and use it to filter revisions?

navidshaikh · 2019-07-03T09:42:50Z

Maybe we should generally increase the default timeout to something like 2 mins ?

One of the candidate to land in kn config, but yes the default timeout can be increased. Another option, we can set --wait-timeout in create calls for CI (as it seems the success rate of e2e tests > flakiness).

rhuss · 2019-07-08T17:33:29Z

@savitaashture #134 has been merged. Could you please rebase ?

savitaashture · 2019-07-08T19:13:44Z

@savitaashture #134 has been merged. Could you please rebase ?

Done

navidshaikh · 2019-07-09T16:11:31Z

+	} else if !action.Matches("list", "services") {
+		t.Errorf("Bad action %v", action)
+	}
+	assert.Check(t, util.ContainsAll(output[0], "NAME", "DOMAIN", "GENERATION", "AGE", "CONDITIONS", "READY", "REASON"))


The DOMAIN field is being replaced in #247 with URL, one of these two PRs need to rebase and update based on which gets merged first.

This PR is around since a while, probably we can consider updating #247 once this is in.
WDYT @rhuss ?

I'm fine with either. However I suggest to merge this PR first, and then update #247 based on this.

googlebot · 2019-07-09T16:37:40Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

ℹ️ Googlers: Go here for more info.

rhuss

looks good, thanks ! Minor nit about naming, otherwise we are good to merge.

rhuss · 2019-07-10T06:10:20Z

+	} else if !action.Matches("list", "services") {
+		t.Errorf("Bad action %v", action)
+	}
+	assert.Check(t, util.ContainsAll(output[0], "NAME", "DOMAIN", "GENERATION", "AGE", "CONDITIONS", "READY", "REASON"))


I'm fine with either. However I suggest to merge this PR first, and then update #247 based on this.

rhuss · 2019-07-10T07:09:43Z

@savitaashture if you update the docs a bit, could you also please squash the PR to a single commit and force push it again ? I hope that we then get rid of the CLA issue so that we can merge that PR. Thanks !

googlebot · 2019-07-10T07:30:58Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

knative-metrics-robot · 2019-07-10T07:32:48Z

The following is the coverage report on pkg/.
Say /test pull-knative-client-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/kn/commands/revision/revision_list.go	80.6%	85.7%	5.2
pkg/kn/commands/service/service_list.go	79.2%	86.7%	7.5

rhuss · 2019-07-10T08:26:16Z

/approve
/lgtm

knative-prow-robot · 2019-07-10T08:28:38Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rhuss, savitaashture

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [rhuss]

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

googlebot added the cla: yes Indicates the PR's author has signed the CLA. label May 30, 2019

knative-prow-robot requested review from evankanderson and mattmoor May 30, 2019 11:34

knative-prow-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 30, 2019

knative-prow-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label May 30, 2019

rhuss reviewed May 30, 2019

View reviewed changes

Comment thread pkg/kn/commands/revision_get.go Outdated

rhuss reviewed May 30, 2019

View reviewed changes

savitaashture force-pushed the issue-143 branch 2 times, most recently from b1593c5 to a3fc452 Compare May 31, 2019 07:50

rhuss mentioned this pull request Jun 2, 2019

The road to 0.2.0 #158

Closed

savitaashture force-pushed the issue-143 branch from a3fc452 to dcf4cfa Compare June 10, 2019 09:00

knative-prow-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jun 10, 2019

savitaashture force-pushed the issue-143 branch 5 times, most recently from d9bcedb to 8eba6dd Compare June 14, 2019 19:07

cppforlife reviewed Jun 17, 2019

View reviewed changes

Comment thread pkg/kn/commands/revision/revision_list.go Outdated

cppforlife reviewed Jun 17, 2019

View reviewed changes

Comment thread pkg/kn/commands/revision/revision_list.go Outdated

savitaashture force-pushed the issue-143 branch from 8eba6dd to 718b3ea Compare June 18, 2019 18:46

navidshaikh reviewed Jun 19, 2019

View reviewed changes

Comment thread pkg/kn/commands/revision/revision_list_test.go Outdated

rhuss added this to the v0.2.0 milestone Jul 8, 2019

savitaashture force-pushed the issue-143 branch from 40c1831 to 4bc0b8e Compare July 8, 2019 19:11

knative-prow-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jul 8, 2019

navidshaikh reviewed Jul 9, 2019

View reviewed changes

savitaashture force-pushed the issue-143 branch from 4bc0b8e to 3ecad95 Compare July 9, 2019 16:35

googlebot added cla: no Indicates the PR's author has not signed the CLA. and removed cla: yes Indicates the PR's author has signed the CLA. labels Jul 9, 2019

navidshaikh reviewed Jul 9, 2019

View reviewed changes

Comment thread pkg/kn/commands/service/service_list_test.go Outdated

savitaashture force-pushed the issue-143 branch from 3ecad95 to 3902b02 Compare July 9, 2019 17:45

rhuss suggested changes Jul 10, 2019

View reviewed changes

get a particular service and revision based on name parameter

7a8d6a2

savitaashture force-pushed the issue-143 branch from 3902b02 to 7a8d6a2 Compare July 10, 2019 07:30

googlebot added cla: yes Indicates the PR's author has signed the CLA. and removed cla: no Indicates the PR's author has not signed the CLA. labels Jul 10, 2019

knative-prow-robot assigned rhuss Jul 10, 2019

knative-prow-robot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jul 10, 2019

rhuss approved these changes Jul 10, 2019

View reviewed changes

knative-prow-robot merged commit 3eb6e41 into knative:master Jul 10, 2019

rhuss mentioned this pull request Jul 10, 2019

kn service get <service-name> will list all the services #143

Closed

Conversation

savitaashture commented May 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

knative-prow-robot commented May 30, 2019

Uh oh!

rhuss May 30, 2019

Choose a reason for hiding this comment

Uh oh!

savitaashture May 31, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rhuss left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rhuss May 30, 2019

Choose a reason for hiding this comment

Uh oh!

savitaashture May 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

savitaashture commented May 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rhuss commented Jun 6, 2019

Uh oh!

savitaashture commented Jun 10, 2019

Uh oh!

rhuss commented Jun 10, 2019

Uh oh!

navidshaikh commented Jun 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

savitaashture commented Jun 14, 2019

Uh oh!

Uh oh!

Uh oh!

navidshaikh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

navidshaikh commented Jul 3, 2019

Uh oh!

rhuss commented Jul 8, 2019

Uh oh!

savitaashture commented Jul 8, 2019

Uh oh!

navidshaikh Jul 9, 2019

Choose a reason for hiding this comment

Uh oh!

rhuss Jul 10, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

googlebot commented Jul 9, 2019

Uh oh!

Uh oh!

rhuss left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

rhuss Jul 10, 2019

Choose a reason for hiding this comment

Uh oh!

rhuss commented Jul 10, 2019

Uh oh!

googlebot commented Jul 10, 2019

Uh oh!

knative-metrics-robot commented Jul 10, 2019

Uh oh!

rhuss commented Jul 10, 2019

Uh oh!

knative-prow-robot commented Jul 10, 2019

savitaashture commented May 30, 2019 •

edited

Loading

savitaashture May 31, 2019 •

edited

Loading

savitaashture commented May 31, 2019 •

edited

Loading

navidshaikh commented Jun 14, 2019 •

edited

Loading