Skip to content

Fix lint and CLI Integration Test errors from PR#258 - #273

Closed
Prathibha Muralidharan (Prathibha-m) wants to merge 6 commits into
masterfrom
lint_cli
Closed

Fix lint and CLI Integration Test errors from PR#258#273
Prathibha Muralidharan (Prathibha-m) wants to merge 6 commits into
masterfrom
lint_cli

Conversation

@Prathibha-m

Copy link
Copy Markdown
Contributor

No description provided.

@Prathibha-m Prathibha Muralidharan (Prathibha-m) changed the title Fix lint erros from PR#258 Fix lint errors from PR#258 Aug 28, 2019
@Prathibha-m Prathibha Muralidharan (Prathibha-m) changed the title Fix lint errors from PR#258 Fix lint and CLI Integration Test errors from PR#258 Aug 28, 2019
Comment thread cmd/lint/main.go
linter.ExcludeCommandContains("iam rolebinding"),
// skip secret commands
linter.ExcludeCommandContains("secret"),
linter.ExcludeCommandContains("schema-registry enable"),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm should probably just be excluding as few as possible here

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've excluded commands which don't use names/ID's (which is pretty much all of them)

@codecov

codecov Bot commented Aug 28, 2019

Copy link
Copy Markdown

Codecov Report

❗ No coverage uploaded for pull request base (master@173f34d). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #273   +/-   ##
=========================================
  Coverage          ?   43.05%           
=========================================
  Files             ?       63           
  Lines             ?     4775           
  Branches          ?        0           
=========================================
  Hits              ?     2056           
  Misses            ?     2457           
  Partials          ?      262
Impacted Files Coverage Δ
internal/cmd/schema-registry/command.go 70.16% <100%> (ø)
internal/cmd/schema-registry/command_subject.go 86.04% <100%> (ø)
internal/cmd/schema-registry/command_schema.go 53.04% <100%> (ø)

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 173f34d...2dc23fd. Read the comment docs.

@codyaray Cody A. Ray (codyaray) left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to step back and review the DevX here. It's inconsistent with the rest of the CLI.

Comment thread cmd/lint/main.go Outdated
vocabWords = []string{
"ccloud", "kafka", "api", "acl", "url", "config", "multizone", "transactional", "ksql", "decrypt", "iam", "rolebinding",
"geo",
"geo", "subject", "version", "schema",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these are actual english words. why do they need to be added here?

Comment thread internal/cmd/schema-registry/command.go Outdated
createCmd.Flags().String("cloud", "", "Cloud provider (e.g. 'aws', 'azure', or 'gcp').")
_ = createCmd.MarkFlagRequired("cloud")
createCmd.Flags().String("geo", "", "Either 'us', 'eu', or 'apac' (only applies to Enterprise accounts)")
createCmd.Flags().String("geo", "", "Either 'us', 'eu', or 'apac' (only applies to Enterprise accounts).")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this "only applies to..." still accurate?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As per the 1-pager doc, that's what it says. But I'll check again!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will change it. Thanks for catching that!

Comment thread cmd/lint/main.go Outdated
linter.OnlyLeafCommands, linter.ExcludeCommand(utilityCommands...),
// skip resource container commands
linter.ExcludeUse("list", "auth"),
linter.ExcludeUse("list", "auth", "describe", "enable"),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a very large blanket exclude. this already tests all the other "describe" commands (since that's a standard verb). I'm not sure about "enable" but it sounds pretty standard and shouldn't be wholeheartedly excluded like this.

(Both describe and enable refer to a single resource, right? That's why this exclusion doesn't make so much since, like "list" which doesn't refer to a single resource)

@Prathibha-m Prathibha Muralidharan (Prathibha-m) Aug 29, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see your point. Would it then be a better idea to simply have linter.ExcludeCommandContains("schema-registry"), since schema-registry commands do not have a name or id passed in

Comment thread cmd/lint/main.go Outdated
// skip following schema-registry commands which do not use names/ID's
linter.ExcludeCommandContains("schema-registry compatibility"),
linter.ExcludeCommandContains("schema-registry schema"),
linter.ExcludeCommandContains("schema-registry mode"),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think I was around to review the original PRs, but these don't seem to all fit with expected grammar for the CLI:
schema-registry <resource--noun> <standard verb: create/describe/update/delete/etc>

I would expect it to be more like this to fit with the rest of the CLI:

schema-registry cluster enable

schema-registry cluster describe
# describe should include the mode and compatibility level for the cluster

schema-registry cluster update --mode=READWRITE

schema-registry cluster update --compatibility=BACKWARD

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants