CC-4669 CC-4671 CLI-125 Schema Registry API Key commands - #258
Merged
Prathibha Muralidharan (Prathibha-m) merged 45 commits intoAug 28, 2019
Conversation
Prathibha Muralidharan (Prathibha-m)
requested a review
from a team
as a code owner
August 19, 2019 17:20
Prathibha Muralidharan (Prathibha-m)
force-pushed
the
prathibha-sr-apikey
branch
2 times, most recently
from
August 20, 2019 18:06
4604514 to
c672dae
Compare
Codecov Report
@@ Coverage Diff @@
## master #258 +/- ##
==========================================
+ Coverage 41.08% 43.04% +1.96%
==========================================
Files 62 63 +1
Lines 4732 4776 +44
==========================================
+ Hits 1944 2056 +112
+ Misses 2543 2458 -85
- Partials 245 262 +17
Continue to review full report at Codecov.
|
Prathibha Muralidharan (Prathibha-m)
force-pushed
the
prathibha-sr-apikey
branch
from
August 20, 2019 18:22
c672dae to
e87e941
Compare
Prathibha Muralidharan (Prathibha-m)
requested a review
from Cody A. Ray (codyaray)
August 20, 2019 19:50
Prathibha Muralidharan (Prathibha-m)
changed the title
Cody A. Ray (codyaray)
left a comment
Cody A. Ray (codyaray)
left a comment
Contributor
There was a problem hiding this comment.
approach looks good to me. a bunch of little code nits and a couple missing tests, but no blockers in here
|
|
||
| func (suite *APITestSuite) TestDeleteApiKey() { | ||
| cmd := suite.newCMD() | ||
| cmd.SetArgs(append([]string{"delete", apiKey})) |
Contributor
There was a problem hiding this comment.
probably want to verify that correct apiKey ID was passed to the update and delete api keys
Prathibha Muralidharan (Prathibha-m)
force-pushed
the
prathibha-sr-apikey
branch
3 times, most recently
from
August 23, 2019 21:02
dc2960d to
7b85955
Compare
Prathibha Muralidharan (Prathibha-m)
force-pushed
the
prathibha-sr-apikey
branch
from
August 27, 2019 20:11
5afb8c0 to
f44374f
Compare
Cody A. Ray (codyaray)
approved these changes
Aug 27, 2019
Cody A. Ray (codyaray)
left a comment
Cody A. Ray (codyaray)
left a comment
Contributor
There was a problem hiding this comment.
lgtm, nice work!
a couple small things left, mostly test updates and a couple nits.
|
|
||
| if err := c.keystore.StoreAPIKey(userKey, kcc.ID, environment); err != nil { | ||
| return errors.HandleCommon(errors.Wrapf(err, "Unable to store API key locally."), cmd) | ||
| if resourceType == "kafka" { |
Contributor
There was a problem hiding this comment.
can you pull out consts to the top of the file for the different resource types?
| } | ||
| return &kafkav1.KafkaCluster{AccountId: c.Config.Auth.Account.Id, Id: kafka.ID, ApiEndpoint: kafka.APIEndpoint}, nil | ||
| } | ||
| func (c *ConfigHelper) SchemaRegistry(resourceID, environment string, ctx context.Context) (*srv1.SchemaRegistryCluster, error) { |
Contributor
There was a problem hiding this comment.
just noticed this has nothing to do with the CLI config; why's it in the ConfigHelper?
nit: ctx should always be the first param on a method
Contributor
Author
|
Thanks so much for the reviews Cody A. Ray (@codyaray)! Addressed all your comments |
Prathibha Muralidharan (Prathibha-m)
force-pushed
the
prathibha-sr-apikey
branch
from
August 28, 2019 19:44
6c5fe99 to
d80f6d1
Compare
Prathibha Muralidharan (Prathibha-m)
added a commit
that referenced
this pull request
Aug 28, 2019
This reverts commit 173f34d.
Contributor
|
I am going to revert this PR for now since #273 which fixes it requires some additional work / renaming which won't happen for a few days |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.