Skip to content

feat: ability to set and modify the default project#287

Merged
aeneasr merged 18 commits intomasterfrom
hf-use-default-project
Jan 18, 2023
Merged

feat: ability to set and modify the default project#287
aeneasr merged 18 commits intomasterfrom
hf-use-default-project

Conversation

@CaptainStandby
Copy link
Copy Markdown
Contributor

@CaptainStandby CaptainStandby commented Dec 28, 2022

Adds the new command use project to the cli that can be used to manipulate the selected project.

Examples

  1. Calling use without id will echo the currently selected project id
$ ory use project
ID      121035f0-5d74-472b-8cc3-39fd727fdfd2
  1. Calling use with id will set the currently selected project
$ ory use project c2e150b3-63cf-4aeb-998e-8250a748c108
ID      c2e150b3-63cf-4aeb-998e-8250a748c108
  1. Calling a command that previously required a project id will now use the selected project when no id is provided
$ ory get project
ID      121035f0-5d74-472b-8cc3-39fd727fdfd2
SLUG    zen-napier-otwc45scr7
STATE   running
NAME    MyProject
  1. When creating a new project the newly created project was previously set as the selected project. This has been changed to only set the selected project when --use-project is provided or the currently selected project is invalid.
$ ory create project --name doIt --use-project
Project created successfully!
ID      c0b9df27-4650-4c7b-9c29-37f499503d6a
SLUG    nervous-allen-47ubo7x9ew
STATE   running
NAME    doIt

$ ory use project
ID      c0b9df27-4650-4c7b-9c29-37f499503d6a

$ ory create project --name no-use              
Project created successfully!
ID      81f9df40-8a45-45e0-9d35-15a67f9840c5
SLUG    lucid-ride-guygrtm16y
STATE   running
NAME    no-use

$ ory use project
ID      c0b9df27-4650-4c7b-9c29-37f499503d6a

Related Issue or Design Document

closes #154

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

@CaptainStandby CaptainStandby self-assigned this Dec 28, 2022
@CaptainStandby CaptainStandby changed the title feat: Ability to set and modify the default project feat: ability to set and modify the default project Dec 28, 2022
@CaptainStandby CaptainStandby marked this pull request as ready for review December 28, 2022 19:34
@aeneasr
Copy link
Copy Markdown
Member

aeneasr commented Dec 29, 2022

I have not done a full code review yet, but from a high level this looks very good :) Nice job! I will try to do a detailed code review in the next days

@aeneasr
Copy link
Copy Markdown
Member

aeneasr commented Jan 2, 2023

When creating a new project the newly created project was previously set as the selected project. This has been changed to only set the selected project when --use-project is provided or the currently selected project is invalid.

In the documentation, we will probably need to use this flag everywhere, because we often would do:

ory create project --use-project --name ...
ory ...

In my mind, when I create a project, I usually want to continue modifying it. My proposal would be to reverse this logic:

ory create project --dont-use

WDYT?

@CaptainStandby
Copy link
Copy Markdown
Contributor Author

When creating a new project the newly created project was previously set as the selected project. This has been changed to only set the selected project when --use-project is provided or the currently selected project is invalid.

In the documentation, we will probably need to use this flag everywhere, because we often would do:

ory create project --use-project --name ...
ory ...

In my mind, when I create a project, I usually want to continue modifying it. My proposal would be to reverse this logic:

ory create project --dont-use

WDYT?

I'm not a big fan of implicit side effects as in my experience they often tend to cause confusion and unexpected behaviour. For the documentation I would probably recommend not to rely on the "current project" logic at all and always be specific for this exact reason.

For the special case when a project is created and no default was set, the default is implicitly set to the newly created project, which is fine in this context.

@aeneasr
Copy link
Copy Markdown
Member

aeneasr commented Jan 3, 2023

Point made, point taken :)

@CaptainStandby CaptainStandby force-pushed the hf-use-default-project branch 2 times, most recently from 24aef9d to 0815a3a Compare January 3, 2023 15:58
@CaptainStandby CaptainStandby requested a review from aeneasr January 4, 2023 13:48
@aeneasr
Copy link
Copy Markdown
Member

aeneasr commented Jan 5, 2023

@hperl could you maybe help out with a review here? :)

@hperl hperl self-requested a review January 5, 2023 15:36
Copy link
Copy Markdown
Contributor

@hperl hperl left a comment

Choose a reason for hiding this comment

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

This looks really good already 🎉. I especially like the thorough testing 🤩. I added some remarks below.

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.

Great :) Thank you for paying extra attention in the tests! Generally speaking, this looks quite good to me. There are two comments related to Go standards. Similar to Henning, I suggest to reduce the duplication in the tests. Especially for larger tests (such as patch) it's adding way too much bloat.

I left a comment which could help you implement the tests without duplication. We use this pattern already quite extensively in Ory Kratos

@CaptainStandby CaptainStandby changed the title feat: ability to set and modify the default project refactor(e2e): ability to set and modify the default project Jan 13, 2023
@CaptainStandby CaptainStandby changed the title refactor(e2e): ability to set and modify the default project feat: ability to set and modify the default project Jan 13, 2023
Copy link
Copy Markdown
Contributor

@hperl hperl 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, LGTM 🎉

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.

Great job!

@aeneasr aeneasr merged commit d8e6a19 into master Jan 18, 2023
@aeneasr aeneasr deleted the hf-use-default-project branch January 18, 2023 09:26
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.

Ability to set and modify the default project

3 participants