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

Refactor and improve users import cmd #735

Merged
merged 8 commits into from
Apr 13, 2023
Merged

Refactor and improve users import cmd #735

merged 8 commits into from
Apr 13, 2023

Conversation

sergiught
Copy link
Contributor

@sergiught sergiught commented Apr 13, 2023

🔧 Changes

This PR brings various improvements to the users import command. Additional context added within code comments below.

📚 References

🔬 Testing

📝 Checklist

  • All new/changed/fixed functionality is covered by tests (or N/A)
  • I have added documentation for all new/changed functionality (or N/A)

@sergiught sergiught force-pushed the patch/users-import branch 2 times, most recently from 3933e6a to 538b6bf Compare April 13, 2023 12:29
@@ -18,9 +18,15 @@ auth0 users import [flags]
```
auth0 users import
auth0 users import --connection "Username-Password-Authentication"
auth0 users import --connection "Username-Password-Authentication" --users-body "[]"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The biggest change here is the exposure of the users body flag. Previously this was hidden and only used to store the contents of the file opened up through the editor. We're now allowing users to pass the body directly bypassing the template selection. This change also enables us to add automated integration tests and improves DX.

Copy link
Contributor

@Widcket Widcket Apr 13, 2023

Choose a reason for hiding this comment

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

This data should also be able to be passed by pipe, like in other similar commands.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @Widcket 👍🏻 good catch. Updated in 735e93f (#735)

Copy link
Contributor

Choose a reason for hiding this comment

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

The docs should also be updated to include a piped example, like in auth0 rules create or auth0 api.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if editorErr != nil {
return fmt.Errorf("Failed to capture input from the editor: %w", editorErr)
}
if inputs.UsersBody == "" {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we don't pass a body directly, then we're gonna select a template and use the editor.

}

if !confirmed {
return nil
if canPrompt(cmd) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This gets skipped if we use --no-input.


list, err := c.api.Connection.List()
func (c *cli) dbConnectionPickerOptions() ([]string, error) {
list, err := c.api.Connection.List(management.Parameter("strategy", management.ConnectionStrategyAuth0))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We're improving the logic here by directly only retrieving database connections from the API instead of retrieving everything and then filtering them out.

@@ -52,25 +52,6 @@ func TestConnectionsPickerOptions(t *testing.T) {
assert.ErrorContains(t, err, "There are currently no database connections.")
},
},
{
name: "no database connections",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

No longer needed as we retrieve only database conns directly from the API.

@sergiught sergiught marked this pull request as ready for review April 13, 2023 12:33
@sergiught sergiught requested a review from a team as a code owner April 13, 2023 12:33
@sergiught sergiught requested a review from Widcket April 13, 2023 12:57
@codecov-commenter
Copy link

codecov-commenter commented Apr 13, 2023

Codecov Report

Patch coverage: 57.74% and project coverage change: +0.22 🎉

Comparison is base (59a0cad) 70.60% compared to head (de8a9a4) 70.82%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #735      +/-   ##
==========================================
+ Coverage   70.60%   70.82%   +0.22%     
==========================================
  Files          87       87              
  Lines       11197    11210      +13     
==========================================
+ Hits         7906     7940      +34     
+ Misses       2778     2750      -28     
- Partials      513      520       +7     
Impacted Files Coverage Δ
internal/cli/users.go 74.63% <57.74%> (+5.19%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

LongForm: "template-body",
userImportBody = Flag{
Name: "Users Body",
LongForm: "users-body",
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a flag name pattern we use in the CLI ("resource-property"), for this kind of input?

For the rules script, we use script:
Screenshot 2023-04-13 at 9 54 31 AM

For the data passed to the api command, we use data:
Screenshot 2023-04-13 at 9 55 57 AM

Copy link
Contributor Author

@sergiught sergiught Apr 13, 2023

Choose a reason for hiding this comment

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

Thanks for raising this. I actually think we're lacking a bit of consistency in these areas. Would you suggest just naming this users so it's clear what kind of content it is? Or perhaps rename the others to rules-script for example?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that we should prefer shorter flag names wherever possible, for usability reasons. Also I suspect changing this pattern to "resources-property" would cause a bigger inconsistency, requiring the renaming of a lot more flags.

users is fine for this, I think.

Copy link
Contributor Author

@sergiught sergiught Apr 13, 2023

Choose a reason for hiding this comment

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

Renamed it to users in c092246 (#735).

But I also removed the short form as it was conflicting and replacing it with anything else than u doesn't really work so well.

Copy link
Contributor

Choose a reason for hiding this comment

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

Consider having -u be the short form for users and -U the short version for upsert.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point 👍🏻 de8a9a4 (#735)

For some reason I thought short forms had to always be lower case.

@sergiught sergiught requested a review from Widcket April 13, 2023 13:23
@sergiught sergiught force-pushed the patch/users-import branch 3 times, most recently from 527cce8 to 6caee69 Compare April 13, 2023 16:21
@sergiught sergiught force-pushed the patch/users-import branch from 6caee69 to 19b6a0c Compare April 13, 2023 16:32
Copy link
Contributor

@willvedd willvedd left a comment

Choose a reason for hiding this comment

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

Large improvement!

@sergiught sergiught merged commit be546f2 into main Apr 13, 2023
@sergiught sergiught deleted the patch/users-import branch April 13, 2023 17:22
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