CC-6178: Modify SR CLI grammar by Prathibha-m · Pull Request #278 · confluentinc/cli

Prathibha Muralidharan (Prathibha-m) · 2019-09-04T20:17:53Z

This PR changes command usage grammar to
schema-registry cluster enable
schema-registry cluster describe
schema-registry cluster update --mode=READWRITE
schema-registry cluster update --compatibility=BACKWARD

schema-registry subject describe subjectname
schema-registry subject update subjectname --mode=READWRITE
schema-registry subject update subjectname --compatibility=BACKWARD

This PR also contains commits from #258 for
api-key create --resource=resource-id
api-key list --resource=resource-id

JIRA issue: https://confluentinc.atlassian.net/browse/CC-6178

1-pager: https://confluentinc.atlassian.net/wiki/spaces/PM/pages/878445212/1-pager+CLI+CCloud+Schema+Registry+support

Slack conversation: https://confluent.slack.com/archives/C9Y6NAM6X/p1567115005035600

codecov · 2019-09-04T20:26:34Z

Codecov Report

Merging #278 into lint_cli will decrease coverage by 0.16%.
The diff coverage is 72.34%.

@@             Coverage Diff              @@
##           lint_cli     #278      +/-   ##
============================================
- Coverage     43.05%   42.88%   -0.17%     
============================================
  Files            63       62       -1     
  Lines          4775     4689      -86     
============================================
- Hits           2056     2011      -45     
+ Misses         2457     2422      -35     
+ Partials        262      256       -6

Impacted Files	Coverage Δ
internal/cmd/schema-registry/command_schema.go	`53.57% <ø> (+0.52%)`	⬆️
internal/cmd/schema-registry/command.go	`100% <100%> (+29.83%)`	⬆️
internal/cmd/schema-registry/utils.go	`66.66% <66.66%> (+29.16%)`	⬆️
internal/cmd/schema-registry/command_cluster.go	`72.18% <72.18%> (ø)`
internal/cmd/schema-registry/command_subject.go	`77.39% <72.97%> (-8.66%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2dc23fd...ad68d4b. Read the comment docs.

codecov · 2019-09-04T20:26:35Z

Codecov Report

Merging #278 into master will increase coverage by 2.33%.
The diff coverage is 65.77%.

@@            Coverage Diff             @@
##           master     #278      +/-   ##
==========================================
+ Coverage   41.02%   43.36%   +2.33%     
==========================================
  Files          62       62              
  Lines        4739     4707      -32     
==========================================
+ Hits         1944     2041      +97     
+ Misses       2550     2400     -150     
- Partials      245      266      +21

Impacted Files	Coverage Δ
internal/pkg/errors/errors.go	`12.5% <ø> (ø)`	⬆️
internal/pkg/cmd/config.go	`19.51% <ø> (ø)`	⬆️
internal/pkg/cmd/flags.go	`0% <0%> (ø)`	⬆️
internal/cmd/command.go	`0% <0%> (ø)`	⬆️
internal/cmd/schema-registry/command.go	`100% <100%> (+29.83%)`	⬆️
internal/cmd/apikey/utils.go	`47.61% <47.61%> (ø)`
internal/cmd/apikey/command.go	`56.59% <70.37%> (+56.59%)`	⬆️
internal/cmd/schema-registry/command_cluster.go	`70.7% <70.7%> (ø)`
internal/cmd/schema-registry/command_subject.go	`77.39% <73.75%> (-8.66%)`	⬇️
internal/cmd/schema-registry/utils.go	`69.44% <75%> (+31.94%)`	⬆️
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1f0e8ad...43f330e. Read the comment docs.

Cody A. Ray (codyaray)

looking pretty good. mostly small stuff

Cody A. Ray (codyaray) · 2019-09-09T19:03:41Z

-		Key         string
-		Description string
-		UserId      int32
+	apiKeys, err = c.client.List(context.Background(), &authv1.ApiKey{AccountId: accId, LogicalClusters: []*authv1.ApiKey_Cluster{{Id: clusterId, Type: resourceType}}})


i was reviewing this code with Chris Sreesangkom (@csreesan) the other day and realized that this API call probably doesn't actually work with the real API server today. Can you confirm that you tested this manually?

this isn't sending the cluster_id field to the API correctly, i believe

I did manually test it.. api-key list prints something like

dist/ccloud/ccloud_darwin_amd64/ccloud api-key list --resource lsrc-dgq91

Key | Owner | Description

+--------------------+-------+-------------+

DULRGWWC46UVAUMI | 10875 |

In the unit test, we cover if cluster_id is being sent to the API. Should I be checking elsewhere?

Interesting. I don't understand how/where this is being sent from then. Chris just merged this, which I think fixes this issue: https://github.com/confluentinc/ccloud-sdk-go/pull/58

But if the issue doesn't actually exist... ?

I do remember using List a week ago before I pushed the initial changes. Not sure what could've happened in between 🤔

but list didn't use to filter by clusterId, right?

that's what Chris Sreesangkom (@csreesan) is adding. Its in PR now though, should be done soon.

Oh, right. I see what you mean! Yeah it didn't

Cody A. Ray (codyaray) · 2019-09-12T14:18:05Z

-			ID:  cluster.Id,
-			URL: cluster.Endpoint,
-		}
+		_ = printer.RenderTableOut(cluster, []string{"Id", "Endpoint"}, map[string]string{"ID": "Cluster ID", "URL": "Endpoint URL"}, os.Stdout)


you can move these to the top of the file and call them enableLabels and enableRenames so you don't have to duplicate them twice

Cody A. Ray (codyaray)

This looks good to me now!

I didn't go through the full checklist of UX changes we discussed, but the code it looks like its doing all the right things.

I think as long as you update to ccloud-sdk-go v0.0.12 and do a full end to end test again, I'm happy! :)

Pass AccountID from environment to create API key Mark current context API key in list of API keys Revert changes to API key delete Remove extra import WIP API key tests WIP Refactored Fix tests Refactor and cleanup Pass account ID to retrieve SR correctly update docs description Error handling and Cleanup fmt Add parameter for flag in GetKafkaConfig Address comments Remove unnecessary err check Modify integration test lint Locl commit Clean up Remove unit test (temp) Fix tests Fix tests i WIP Remove unit test . lint Modify resource scoped commands Add unit test Cleanup Integration test Clean fix WIP WIP Integration test clean wip Fix IT Clean up Fix fixture i nit Error test case Fixture spacing Remove ID check address comments Rebase on master Address comments Mock tests Improve unit test validation

nits Hide SR

# This is the 1st commit message: Add subject commands # This is the commit message #2: Docs Unhide SR make deps

Typo

Address comments Update comment Mark geo flag required

Prathibha Muralidharan (Prathibha-m) · 2019-09-12T17:44:16Z

Thanks Cody A. Ray (@codyaray) for the review! :) Updated ccloud sdk and ran the tests.

fmt Fmt

Prathibha Muralidharan (Prathibha-m) requested a review from a team as a code owner September 4, 2019 20:17

Prathibha Muralidharan (Prathibha-m) changed the base branch from lint_cli to prathibha-sr-apikey September 4, 2019 20:30

Prathibha Muralidharan (Prathibha-m) force-pushed the subject_command branch 2 times, most recently from 2d95308 to 68519f3 Compare September 4, 2019 20:58

Prathibha Muralidharan (Prathibha-m) requested review from David Hyde (DABH), Cody A. Ray (codyaray) and Cyrus Vafadari (cyrusv) September 4, 2019 21:33

Prathibha Muralidharan (Prathibha-m) force-pushed the subject_command branch from c7431ee to 8ea6146 Compare September 4, 2019 23:40

Prathibha Muralidharan (Prathibha-m) changed the base branch from prathibha-sr-apikey to master September 4, 2019 23:40

Prathibha Muralidharan (Prathibha-m) force-pushed the subject_command branch from c645dc0 to 9189e3b Compare September 5, 2019 00:14

Cody A. Ray (codyaray) reviewed Sep 9, 2019

View reviewed changes

Prathibha Muralidharan (Prathibha-m) force-pushed the subject_command branch 3 times, most recently from b605699 to a00f6a4 Compare September 10, 2019 18:12

Cody A. Ray (codyaray) reviewed Sep 10, 2019

View reviewed changes

Comment thread internal/cmd/schema-registry/command_cluster.go Outdated

Cody A. Ray (codyaray) reviewed Sep 10, 2019

View reviewed changes

Comment thread internal/cmd/schema-registry/command_cluster.go Outdated

Cody A. Ray (codyaray) reviewed Sep 10, 2019

View reviewed changes

Comment thread internal/cmd/schema-registry/command_cluster_test.go Outdated

Cody A. Ray (codyaray) reviewed Sep 12, 2019

View reviewed changes

Cody A. Ray (codyaray) approved these changes Sep 12, 2019

View reviewed changes

Prathibha Muralidharan (Prathibha-m) and others added 9 commits September 12, 2019 09:40

Unhide SR

4d13d68

nits Hide SR

Fix lint errors from Schema Registry

92342fe

Cluster command updates

aa3676a

# This is a combination of 2 commits.

dddb4f2

# This is the 1st commit message: Add subject commands # This is the commit message #2: Docs Unhide SR make deps

More schema tests

f331a47

WIP integration test

1a41d99

Typo

minor:import order

d321ff0

address comments

6c53d0a

Address comments Update comment Mark geo flag required

Prathibha Muralidharan (Prathibha-m) added 5 commits September 12, 2019 09:44

Add logger

976a3fd

make fmt

01c10cb

Add CLIName replacement to docs

d125e4f

Error checks

4b7670b

create variables for reuse

9a100d9

Prathibha Muralidharan (Prathibha-m) force-pushed the subject_command branch 3 times, most recently from 1f20665 to b6e391e Compare September 12, 2019 17:43

rebase on master

43f330e

fmt Fmt

Prathibha Muralidharan (Prathibha-m) force-pushed the subject_command branch from b6e391e to 43f330e Compare September 12, 2019 17:54

Prathibha Muralidharan (Prathibha-m) merged commit 1842f37 into master Sep 12, 2019

Prathibha Muralidharan (Prathibha-m) deleted the subject_command branch September 23, 2019 22:58

Uh oh!

Conversation

Prathibha Muralidharan (Prathibha-m) commented Sep 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Sep 4, 2019

Codecov Report

Uh oh!

codecov Bot commented Sep 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Cody A. Ray (codyaray) left a comment

Choose a reason for hiding this comment

Uh oh!

Cody A. Ray (codyaray) Sep 9, 2019

Choose a reason for hiding this comment

Uh oh!

Prathibha Muralidharan (Prathibha-m) Sep 9, 2019

Choose a reason for hiding this comment

Uh oh!

Cody A. Ray (codyaray) Sep 10, 2019

Choose a reason for hiding this comment

Uh oh!

Prathibha Muralidharan (Prathibha-m) Sep 10, 2019

Choose a reason for hiding this comment

Uh oh!

Cody A. Ray (codyaray) Sep 10, 2019

Choose a reason for hiding this comment

Uh oh!

Prathibha Muralidharan (Prathibha-m) Sep 10, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Cody A. Ray (codyaray) Sep 12, 2019

Choose a reason for hiding this comment

Uh oh!

Cody A. Ray (codyaray) left a comment • 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!

Prathibha Muralidharan (Prathibha-m) commented Sep 12, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Prathibha Muralidharan (Prathibha-m) commented Sep 4, 2019 •

edited

Loading

codecov Bot commented Sep 4, 2019 •

edited

Loading

Cody A. Ray (codyaray) left a comment •

edited

Loading