Skip to content

feat: project flag accepts slug#291

Merged
zepatrik merged 13 commits intomasterfrom
project-slug-flag
Jan 11, 2023
Merged

feat: project flag accepts slug#291
zepatrik merged 13 commits intomasterfrom
project-slug-flag

Conversation

@tilschuenemann
Copy link
Copy Markdown
Contributor

@tilschuenemann tilschuenemann commented Jan 5, 2023

This PR enables supplying slugs to cmd subcmd project PROJECTORSLUG / cmd ls identities PROJECTORSLUG.

Related Issue or Design Document

closes #267
closes #156

Checklist

  • I have read the contributing guidelines
    and signed the CLA.
  • I have referenced an issue containing the design document if my change
    introduces a new feature.
  • I have read the security policy.
  • I confirm that this pull request does not address a security
    vulnerability. If this pull request addresses a security vulnerability, I
    confirm that I got approval (please contact
    security@ory.sh) from the maintainers to push
    the changes.
  • I have added tests that prove my fix is effective or that my feature
    works.
  • I have added the necessary documentation within the code base (if
    appropriate).

Further comments

@tilschuenemann tilschuenemann added the area/cli This issue affects Ory's CLI. label Jan 5, 2023
@tilschuenemann tilschuenemann marked this pull request as ready for review January 9, 2023 10:57
@CaptainStandby CaptainStandby linked an issue Jan 10, 2023 that may be closed by this pull request
6 tasks
Copy link
Copy Markdown
Member

@zepatrik zepatrik left a comment

Choose a reason for hiding this comment

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

Nice, I just have some minor improvement suggestions 😉

Comment thread cmd/cloudx/client/handler.go Outdated
Comment on lines +536 to +538
if id == uuid.Nil {
return nil, errors.Errorf("No project selected! Please use the flag --project to specify one.\n")
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I would move this error message to the very top of this function and make the condition projectOrSlug == "". At this point, the proper error should be similar to no project found with slug %s, only slugs %v are known. This makes it a lot easier for users to spot their mistakes.

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.

Thanks for the review! :)

Should the output look like this then or should we only show the closest slug?

no project found with slug romantic-kilby-g76if4751f, only slugs known are: 
romantic-kilby-g76if4751e
busy-jang-bwemahrjn5
busy-black-v1msdqmms0
ecstatic-swartz-ququpvs916
peaceful-kare-ugrr1tziby
stoic-dijkstra-4x7qznhyru

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I would just print the whole list. Maybe you can use the command listing the projects instead? It should at least have a type to show projects as nice tables.

Comment thread cmd/cloudx/client/handler_test.go Outdated
Copy link
Copy Markdown
Member

@zepatrik zepatrik left a comment

Choose a reason for hiding this comment

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

Nice 👍
We should also make sure that in all flag and argument documentation is up to date.

Comment thread cmd/cloudx/client/handler.go Outdated
Comment thread cmd/cloudx/client/handler.go Outdated
tilschuenemann and others added 3 commits January 10, 2023 13:32
Co-authored-by: Patrik <zepatrik@users.noreply.github.com>
Co-authored-by: Patrik <zepatrik@users.noreply.github.com>
Copy link
Copy Markdown
Member

@aeneasr aeneasr left a comment

Choose a reason for hiding this comment

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

Very nice solution :) Looks good to me - @zepatrik WDYT?

@zepatrik zepatrik merged commit 7b71d49 into master Jan 11, 2023
@zepatrik zepatrik deleted the project-slug-flag branch January 11, 2023 10:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/cli This issue affects Ory's CLI.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

The --project flag should accept both UUIDs and slugs Allow using both the slug and ID for get/patch/put projects

3 participants