Extract config logic into own package #743

sergiught · 2023-04-15T23:42:04Z

🔧 Changes

The main focus of this PR is to extract and contain the entire config logic into one package that is fully testable through unit tests, as right now the config logic is spread through multiple places.

Along the way several minor bugs were also fixed:

Always showing the hint to log in to the tenant even if we are logged in when displaying the help for the main command
Edge case where we would end up with a corrupted config file

It is advised to review this PR in 3 steps (commit by commit):

📚 References

https://app.codecov.io/gh/auth0/auth0-cli/pulls (increases test coverage by +0.52%, the badge below failed to update to latest stats)

🔬 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)

codecov-commenter · 2023-04-15T23:53:40Z

Codecov Report

Patch coverage: 67.35% and project coverage change: +0.45 🎉

Comparison is base (e53b3d3) 71.17% compared to head (fc7f35e) 71.63%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #743      +/-   ##
==========================================
+ Coverage   71.17%   71.63%   +0.45%     
==========================================
  Files          87       89       +2     
  Lines       11162    11159       -3     
==========================================
+ Hits         7945     7994      +49     
+ Misses       2700     2661      -39     
+ Partials      517      504      -13

Impacted Files	Coverage Δ
internal/cli/roles_permissions.go	`62.25% <0.00%> (+2.06%)`	⬆️
internal/display/tenants.go	`68.18% <0.00%> (-20.06%)`	⬇️
internal/cli/login.go	`36.63% <22.22%> (+1.51%)`	⬆️
internal/cli/apps.go	`78.63% <25.00%> (ø)`
internal/cli/tenants.go	`63.26% <28.57%> (+1.72%)`	⬆️
internal/cli/cli.go	`45.12% <37.20%> (-14.68%)`	⬇️
internal/cli/utils_shared.go	`55.61% <40.00%> (ø)`
internal/cli/test.go	`46.96% <50.00%> (+2.13%)`	⬆️
internal/config/tenant.go	`52.11% <52.11%> (ø)`
internal/cli/root.go	`90.84% <61.53%> (-1.64%)`	⬇️
... and 8 more

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.

sergiught · 2023-04-17T17:05:24Z

internal/config/tenant.go

+}
+
+// RegenerateAccessToken regenerates the access token for the tenant.
+func (t *Tenant) RegenerateAccessToken(ctx context.Context) error {


This is the only func that doesn't have tests, unfortunately it's extremely hard to test this through unit tests due to lack of sensible abstractions.

Ideally we would refactor the auth and keyring packages next and the entire logic below we could replace with a strategy pattern delegating the actual logic to the auth pkg through an interface that we could mock.

For now I kept this as it was.

sergiught · 2023-04-17T17:06:15Z

internal/cli/login.go

 	}

-	if err := checkInstallID(cli); err != nil {


This logic is now part of the AddTenant func on the Config.

Widcket · 2023-04-17T17:34:42Z

internal/config/config.go

+var ErrNoAuthenticatedTenants = errors.New("Not logged in. Try `auth0 login`.")
+
+// Config holds cli configuration settings.
+type Config struct {


Should we expose an interface with Config's methods, and then use the interface in the cli struct, so that the config can be mocked? It would be useful for testing functions that use config methods like GetTenant(), such as this one: https://github.com/auth0/auth0-cli/blob/main/internal/cli/apps.go#L882

IMO it's unnecessary as right now we can simply inject an in-memory Config struct with any values we might need for testing.

So the setup to test for example the appPickerOptions func would be similar to:

auth0-cli/internal/config/config_test.go

Lines 206 to 234 in de87857

tempFile := createTempConfigFile(t, []byte(`

{

"install_id": "3998b053-dd7f-4bfe-bb10-c4f3a96a0180",

"default_tenant": "auth0-cli.eu.auth0.com",

"tenants": {

"auth0-cli.eu.auth0.com": {

"name": "auth0-cli",

"domain": "auth0-cli.eu.auth0.com",

"access_token": "eyfSaswe",

"expires_at": "2023-04-18T11:18:07.998809Z",

"client_id": "secret"

}

}

}

`))

expectedTenant := Tenant{

Name: "auth0-cli",

Domain: "auth0-cli.eu.auth0.com",

AccessToken: "eyfSaswe",

ExpiresAt: time.Date(2023, time.April, 18, 11, 18, 7, 998809000, time.UTC),

ClientID: "secret",

}

config := &Config{Path: tempFile}

actualTenant, err := config.GetTenant("auth0-cli.eu.auth0.com")

assert.NoError(t, err)

assert.Equal(t, expectedTenant, actualTenant)

That involves creating an actual temp config file. But it certainly does the trick.

Widcket · 2023-04-17T18:23:33Z

internal/cli/apps.go

@@ -479,7 +479,7 @@ func createAppCmd(cli *cli) *cobra.Command {
 				return fmt.Errorf("Unable to create application: %v", err)
 			}

-			if err := cli.setDefaultAppID(a.GetClientID()); err != nil {
+			if err := cli.Config.SaveNewDefaultAppIDForTenant(cli.tenant, a.GetClientID()); err != nil {


Maybe SetDefaultAppForTenant()? Seems a bit verbose. Totally a nit, though. Not a blocker.

Yeah I like that to be honest, I'll do the same with SetDefaultTenant. 👍🏻 Thanks @Widcket

954f34b (#743) I also realized we can set the path to private instead of having it public.

willvedd · 2023-04-18T15:13:14Z

internal/config/config_test.go

+					"auth0-mega-cli.eu.auth0.com": {
+						"name": "auth0-mega-cli",
+						"domain": "auth0-mega-cli.eu.auth0.com",
+						"access_token": "eyfSaswe",
+						"expires_at": "2023-04-18T11:18:07.998809Z",
+						"client_id": "secret"
+					}


Would be nice to add a third tenant here to make it clear what happens if there are multiple "fallbacks".

In Go, maps are unordered collections of key/value pairs. When you range over a map, the order in which the key/value pairs are returned is not guaranteed and can vary between different runs of the program. We can add logic to have a consistent sorting and always set the default tenant to a more predictable one but I'd avoid it IMO as it feels unnecessary. Wdyt?

Reference: https://go.dev/blog/maps Iteration order

willvedd · 2023-04-18T15:23:15Z

internal/config/config_test.go

+}
+
+func TestConfig_SaveNewDefaultTenant(t *testing.T) {
+	t.Run("it can successfully save a new tenant default", func(t *testing.T) {


Reminder: Test case for validating that default tenant actually exists in config.

willvedd · 2023-04-18T15:26:33Z

internal/config/config_test.go

+	})
+}
+
+func TestConfig_IsLoggedInWithTenant(t *testing.T) {


Reminder: Needs test case for access token in keyring.

willvedd · 2023-04-18T15:27:52Z

internal/config/config_test.go

+		}`))
+
+		config := &Config{path: tempFile}
+		assert.False(t, config.IsLoggedInWithTenant(""))


Fix:

Suggested change

assert.False(t, config.IsLoggedInWithTenant(""))

assert.False(t, config.IsLoggedInWithTenant("auth0-cli.eu.auth0.com"))

willvedd

Solid improvement 👍

sergiught force-pushed the patch/config-pkg branch from de49adc to 18973ca Compare April 15, 2023 23:48

sergiught force-pushed the patch/config-pkg branch 4 times, most recently from 2be0048 to 8efc96d Compare April 17, 2023 11:49

Base automatically changed from patch/remove-apps-first-runs-tracking to main April 17, 2023 13:27

sergiught force-pushed the patch/config-pkg branch from 8efc96d to 86f24cd Compare April 17, 2023 16:32

sergiught added 3 commits April 17, 2023 18:42

Add new config pkg

8f75915

Add tests for new config pkg

de87857

Refactor codebase to use new config pkg

8423370

sergiught force-pushed the patch/config-pkg branch from 86f24cd to 8423370 Compare April 17, 2023 16:43

sergiught self-assigned this Apr 17, 2023

sergiught marked this pull request as ready for review April 17, 2023 17:00

sergiught requested a review from a team as a code owner April 17, 2023 17:00

sergiught commented Apr 17, 2023

View reviewed changes

Widcket reviewed Apr 17, 2023

View reviewed changes

sergiught requested review from Widcket and willvedd April 17, 2023 17:44

Widcket reviewed Apr 17, 2023

View reviewed changes

Widcket approved these changes Apr 17, 2023

View reviewed changes

Minor tweaks to naming and setting path to private

954f34b

Widcket approved these changes Apr 18, 2023

View reviewed changes

willvedd reviewed Apr 18, 2023

View reviewed changes

sergiught marked this pull request as draft April 18, 2023 16:47

Add tenant validation to SetDefaultTenant

b95f8bd

sergiught added 7 commits April 18, 2023 18:50

Add args validation when selecting a valid tenant from config

ab6bc8a

Add hint when no tenants available on tenants list

7b12289

Fix config test

aea516e

Add test with keyring access token

7d8d10d

Refactor root command setup

f29bc74

Swap test order

fc7f35e

Add tests for check tenant authentication

ba5f6dd

sergiught marked this pull request as ready for review April 18, 2023 19:39

sergiught requested a review from willvedd April 18, 2023 19:39

willvedd approved these changes Apr 18, 2023

View reviewed changes

sergiught merged commit f6112ce into main Apr 19, 2023

sergiught deleted the patch/config-pkg branch April 19, 2023 09:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extract config logic into own package #743

Extract config logic into own package #743

sergiught commented Apr 15, 2023 •

edited

Loading

codecov-commenter commented Apr 15, 2023 •

edited

Loading

sergiught Apr 17, 2023

sergiught Apr 17, 2023

Widcket Apr 17, 2023

sergiught Apr 17, 2023

Widcket Apr 17, 2023

Widcket Apr 17, 2023 •

edited

Loading

sergiught Apr 18, 2023

sergiught Apr 18, 2023

willvedd Apr 18, 2023

sergiught Apr 18, 2023

willvedd Apr 18, 2023

willvedd Apr 18, 2023

willvedd Apr 18, 2023

willvedd left a comment

	tempFile := createTempConfigFile(t, []byte(`
	{
	"install_id": "3998b053-dd7f-4bfe-bb10-c4f3a96a0180",
	"default_tenant": "auth0-cli.eu.auth0.com",
	"tenants": {
	"auth0-cli.eu.auth0.com": {
	"name": "auth0-cli",
	"domain": "auth0-cli.eu.auth0.com",
	"access_token": "eyfSaswe",
	"expires_at": "2023-04-18T11:18:07.998809Z",
	"client_id": "secret"
	}
	}
	}
	`))

	expectedTenant := Tenant{
	Name: "auth0-cli",
	Domain: "auth0-cli.eu.auth0.com",
	AccessToken: "eyfSaswe",
	ExpiresAt: time.Date(2023, time.April, 18, 11, 18, 7, 998809000, time.UTC),
	ClientID: "secret",
	}

	config := &Config{Path: tempFile}
	actualTenant, err := config.GetTenant("auth0-cli.eu.auth0.com")

	assert.NoError(t, err)
	assert.Equal(t, expectedTenant, actualTenant)

	assert.False(t, config.IsLoggedInWithTenant(""))
	assert.False(t, config.IsLoggedInWithTenant("auth0-cli.eu.auth0.com"))

Extract config logic into own package #743

Extract config logic into own package #743

Conversation

sergiught commented Apr 15, 2023 • edited Loading

🔧 Changes

📚 References

🔬 Testing

📝 Checklist

codecov-commenter commented Apr 15, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Widcket Apr 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

willvedd left a comment

Choose a reason for hiding this comment

sergiught commented Apr 15, 2023 •

edited

Loading

codecov-commenter commented Apr 15, 2023 •

edited

Loading

Widcket Apr 17, 2023 •

edited

Loading