Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CLI-1067 Deprecate input and exposure of numeric ID in admin user, api-key, acl, service-account commands #929

Merged
merged 10 commits into from
Sep 7, 2021

Conversation

MuweiHe
Copy link
Contributor

@MuweiHe MuweiHe commented Jul 20, 2021

Checklist

  1. [CRUCIAL] Is the change for CP or CCloud functionalities that are already live in prod?

    • yes: ok
  2. Did you add/update any commands that accept secrets as args/flags?

    • no: ok

What

These commands should only expose/accept resource ID not numeric ID, but might be still using the corresponding numeric ID for api calls.

References

CCloud CLI ID Format Consistency
CLI-1067 Deprecate use of numeric ID in admin user, api-key, acl, service-account commands

Test&Review

CLI-1067 Sample Output

…unt commands

Only accept/expose resource IDs.
@MuweiHe MuweiHe requested a review from a team as a code owner July 20, 2021 21:28
@MuweiHe MuweiHe requested review from mtodzo and DABH July 20, 2021 21:30
Copy link
Member

@brianstrauch brianstrauch left a comment

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 fully understand the relationship between "users", "user resources" and "service accounts" 😅 so I'll let someone else put the final stamp on this one.

internal/cmd/api-key/command.go Outdated Show resolved Hide resolved
internal/cmd/api-key/command.go Outdated Show resolved Hide resolved
@@ -380,11 +377,12 @@ func (c *command) create(cmd *cobra.Command, _ []string) error {
}

key := &schedv1.ApiKey{
Description: description,
AccountId: c.EnvironmentId(),
UserResourceId: saId,
Copy link
Member

Choose a reason for hiding this comment

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

So a "service account" is a type of "user resource"?

internal/cmd/api-key/command.go Outdated Show resolved Hide resolved
internal/cmd/kafka/command_acl.go Outdated Show resolved Hide resolved
internal/cmd/kafka/command_acl.go Outdated Show resolved Hide resolved
internal/cmd/kafka/command_acl.go Outdated Show resolved Hide resolved
internal/cmd/service-account/command.go Outdated Show resolved Hide resolved
internal/cmd/service-account/command.go Outdated Show resolved Hide resolved
internal/pkg/errors/error_message.go Outdated Show resolved Hide resolved
@brianstrauch
Copy link
Member

@MuweiHe I'm going to update the branch rules for the 2.0 branch, so we don't accidentally merge anything without a review!

internal/cmd/api-key/command.go Show resolved Hide resolved
internal/cmd/api-key/command.go Outdated Show resolved Hide resolved
internal/cmd/api-key/command.go Show resolved Hide resolved
internal/cmd/kafka/command_acl.go Show resolved Hide resolved
@MuweiHe MuweiHe requested a review from mtodzo July 30, 2021 23:11
@DABH
Copy link
Contributor

DABH commented Aug 11, 2021

@mtodzo @brianstrauch can you guys review this one? I think you may have better context than me...

@mtodzo
Copy link
Contributor

mtodzo commented Aug 14, 2021

Made a reminder for myself to re-review this on Monday .. sorry for the delay 😅

@MuweiHe MuweiHe changed the title CLI-1067 Deprecate numeric ID in admin user, api-key, acl, service-account commands CLI-1067 Deprecate input and exposure of numeric ID in admin user, api-key, acl, service-account commands Aug 31, 2021
Copy link
Contributor

@DABH DABH left a comment

Choose a reason for hiding this comment

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

LGTM up to a couple minor comments. Just a general note that we should carefully try to field test 2.0 with internal users etc. as much as we can before handing this off to customers, in case there is any edge case we did not think of. But I appreciate the testing docs and other reference docs for this PR... I am convinced enough to give the +1 :)

internal/cmd/kafka/command_test.go Outdated Show resolved Hide resolved
{args: "kafka acl list --cluster lkc-acls-kafka-api", fixture: "kafka/kafka-acls-list.golden"},
//{args: "kafka acl list --cluster lkc-acls", fixture: "kafka/rp-kafka-acls-list.golden"},
//{args: "kafka acl list --cluster lkc-acls -o json", fixture: "kafka/rp-kafka-acls-list-json.golden"},
//{args: "kafka acl list --cluster lkc-acls -o yaml", fixture: "kafka/rp-kafka-acls-list-yaml.golden"},
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are these commented out?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure who commented out this... Just checked that those 'rp-kafka-acls-list.golden' files no longer exist.

@MuweiHe MuweiHe merged commit 015e187 into 2.0 Sep 7, 2021
@MuweiHe MuweiHe deleted the CLI-1067 branch September 7, 2021 16:57
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.

4 participants