Enable user profile hce 266 #137

JolisaBrownHashiCorp · 2022-11-23T20:13:07Z

🛠️ Description

This PR includes a profile library to enable users setting organization ID, project ID, and development environment on their client configuration instance. With the current changes, users may set profile values via environment variables. Next steps include introducing support for saving profile values optionally in a profile.json file saved in a location chosen by the user.

🔗 External Links

HCE-266 - 'profile' library to handle setting org ID, project ID, environment

👍 Definition of Done

SDK added
SDK updated
Tests added?
Docs updated?

JolisaBrownHashiCorp · 2022-11-28T18:15:39Z

config/hcp.go

-	if c.profile.GetOrganizationID() == "" || c.profile.GetProjectID() == "" {
-		return fmt.Errorf("organization ID and project ID must be non-empty")
-	}
-


Is it best to make organizationID and projectID optional as I'm doing here? Or would it be best to throw an error on client initialization to enforce those fields as a requirement on the config struct? Would it be appropriate as well to allow the environment field to be optional or otherwise assign a default value for the field in the profile if not assigned?

Let's keep these optional. But I would add some validation that ensures that if one of them has a value, the other one also has a value. Since they don't make sense without each other.

bcmdarroch · 2022-11-28T21:44:37Z

config/env.go

+
+	envVarHCPProjectID = "HCP_PROJECT_ID"
+
+	envVarHCPEnvironment = "HCP_ENVIRONMENT"


After some discussion today, let's remove the HCP environment setting from the profile.

bcmdarroch

Excellent work on this! I had a few comments to address, but it's very close.

bcmdarroch · 2022-11-28T22:07:45Z

config/env.go

@@ -52,6 +58,18 @@ const (
 // It will not fail if no or only part of the variables are present.
 func FromEnv() HCPConfigOption {
 	return func(config *hcpConfig) error {
+		// Read user profile information from the environment, the values will only be


I would move this block to the bottom of FromEnv(). The client credentials/authURL checks are more critical to the SDK to function and should fail early, whereas profile variables should always be optional.

bcmdarroch · 2022-11-28T22:10:41Z

config/new.go

@@ -74,6 +75,7 @@ func NewHCPConfig(opts ...HCPConfigOption) (HCPConfig, error) {
 			Scopes:      []string{"openid", "offline_access"},
 		},
 		session: &auth.UserSession{},
+		profile: profile.UserProfile{},


Let's make this a pointer, so we're always mutating the same object if the profile needs to change.

Suggested change

profile: profile.UserProfile{},

profile: &profile.UserProfile{},

bcmdarroch · 2022-11-28T22:12:25Z

config/with.go

+// WithProfile is an option that can be used to provide a custom Profile struct.
+//
+// This should only be necessary for development purposes.
+func WithProfile(organizationID, projectID, hcpEnvironment string) HCPConfigOption {


This seems like a good opportunity for using the Profile interface as a single parameter, so we don't have to keep updating this function signature every time we change what's in the profile.

bcmdarroch · 2022-11-28T22:45:55Z

profile/profile.go

+	hcpEnvironment string
+}
+
+//TODO: Do we need to include comments here or is it covered since we comment the function definitions in the interface?


I think the comments might be more helpful next to their actual function definition, rather than within the interface.

bcmdarroch · 2022-11-28T22:55:50Z

profile/profile.go

+	"os"
+)
+
+type Profile interface {


One more question, more philosophical so I'll leave it up to you. Right now I'm not sure I see the need for a Profile interface. I think we could get away with just the UserProfile struct with the methods defined below. It's true that we could keep the interface and add a mock implementation, the way we did with Session. But where would we use that mock implementation? Would it provide anything useful? This package just seems straightforward enough to not require much code 🤔

Hmmm, I'd think it might be best as well to stick with just using the struct since I'm finding myself maneuvering a bit to find utility for a mock implementation. I agree that since the methods are mostly managing the struct properties, the interface might be a bit overkill. I can go ahead and make that tweak then!

bcmdarroch · 2022-11-29T20:14:24Z

profile/profile.go

+}
+
+func (p *UserProfile) SetOrganizationID(id string) error {
+	err := os.Setenv(envVarHCPOrganizationID, id)


RE: our earlier discussion, we don't want to overwrite environment variables, only read them. Setting the org ID should only involve setting the value in the UserProfile struct.

bcmdarroch · 2022-11-29T21:33:24Z

profile/profile.go

+package profile
+
+const (
+	envVarHCPOrganizationID = "HCP_ORGANIZATION_ID"


These constants are already defined in env.go

bcmdarroch · 2022-11-29T22:12:25Z

config/env.go

+		if hcpOrganizationIDOK && hcpProjectIDOK {
+			userProfile := profile.UserProfile{OrganizationID: hcpOrganizationID, ProjectID: hcpProjectID}
+			if err := apply(config, WithProfile(&userProfile)); err != nil {
+				return fmt.Errorf("failed to set profile fields from environment variables (%s, %s): %w", envVarHCPOrganizationID, envVarHCPProjectID, err)


For consistency with the errors above:

Suggested change

return fmt.Errorf("failed to set profile fields from environment variables (%s, %s): %w", envVarHCPOrganizationID, envVarHCPProjectID, err)

return fmt.Errorf("failed to configure profile based on environment variables (%s, %s): %w", envVarHCPOrganizationID, envVarHCPProjectID, err)

(bonus ask: fix the basd typo I just noticed in those other errors 😅)

bcmdarroch · 2022-11-29T22:14:23Z

config/hcp.go

@@ -84,6 +85,13 @@ type hcpConfig struct {
 	// session is responsible for getting an access token fron our identity provider.
 	// A mock can be used in tests.
 	session auth.Session
+


a bonus for this PR or a follow-up: let's use a Session pointer

bcmdarroch · 2022-11-29T22:15:05Z

config/hcp.go

@@ -84,6 +85,13 @@ type hcpConfig struct {
 	// session is responsible for getting an access token fron our identity provider.
 	// A mock can be used in tests.
 	session auth.Session
+
+	// profile is user's the org id, project id, and application environment


Suggested change

// profile is user's the org id, project id, and application environment

// profile is the user's organization ID and project ID

bcmdarroch · 2022-11-29T22:18:41Z

config/new_test.go

@@ -36,6 +37,8 @@ func TestNew_Options(t *testing.T) {
 		WithPortalURL("https://my-portal:1234"),
 		WithAPI("my-api:2345", &tls.Config{}),
 		WithSCADA("my-scada:3456", &tls.Config{}),
+		// considering that some fields now are optional...should that mean I should be passing in the Profile struct instance directly here (empty or otherwise)?


I think the purpose of this test is to show all the options that are available to be used, so personally I would populate the UserProfile with a project ID and org ID and then assert that the values are set correctly down below.

bcmdarroch · 2022-11-29T22:25:19Z

config/with.go

+
+// WithProfile is an option that can be used to provide a custom UserProfile struct.
+//
+// This should only be necessary for development purposes.


Suggested change

// This should only be necessary for development purposes.

bcmdarroch · 2022-11-29T22:25:44Z

profile/profile.go

+// UserProfile implements the Profile interface.
+type UserProfile struct {
+
+	// hcpOrganizationID is the user's organization ID.


Suggested change

// hcpOrganizationID is the user's organization ID.

// OrganizationID is the user's organization ID.

bcmdarroch · 2022-11-29T22:25:52Z

profile/profile.go

+	// hcpOrganizationID is the user's organization ID.
+	OrganizationID string
+
+	// hcpProjectID is the user's project ID.


Suggested change

// hcpProjectID is the user's project ID.

// ProjectID is the user's project ID.

bcmdarroch · 2022-11-29T22:26:02Z

profile/profile.go

+	ProjectID string
+}
+
+// getOrganizationID() retrieves a user's organization ID.


Suggested change

// getOrganizationID() retrieves a user's organization ID.

// GetOrganizationID() retrieves a user's organization ID.

bcmdarroch · 2022-11-29T22:26:16Z

profile/profile.go

+	return p.OrganizationID
+}
+
+// getProjectID() retrieves the user's project ID.


Suggested change

// getProjectID() retrieves the user's project ID.

// GetProjectID() retrieves the user's project ID.

bcmdarroch · 2022-11-29T22:28:06Z

profile/profile.go

+	envVarHCPProjectID = "HCP_PROJECT_ID"
+)
+
+// UserProfile implements the Profile interface.


This isn't really true anymore, so let's make this comment more descriptive

JolisaBrownHashiCorp added 6 commits November 22, 2022 10:51

create profile fields on hcp config

c29f66c

WIP create profile interface and tests

082706c

refactor profile interface/update path

42471e1

refactor UserProfile struct/update path

7aeaeed

remove deprecated paths

af6f61e

add test coverage

30b5e20

JolisaBrownHashiCorp requested a review from a team November 23, 2022 20:13

JolisaBrownHashiCorp and others added 4 commits November 23, 2022 15:14

update changelog

28d99c7

Remove deprecated test

955401b

resolve linting error

581b100

make org/project id optional

2d61b73

JolisaBrownHashiCorp commented Nov 28, 2022

View reviewed changes

bcmdarroch reviewed Nov 28, 2022

View reviewed changes

bcmdarroch reviewed Nov 29, 2022

View reviewed changes

JolisaBrownHashiCorp added 2 commits November 29, 2022 15:41

replace interface with struct only

2c228f9

require struct pointers instead of object

678715f

bcmdarroch reviewed Nov 29, 2022

View reviewed changes

clean up comments/tweak tests

efb6958

bcmdarroch approved these changes Nov 30, 2022

View reviewed changes

JolisaBrownHashiCorp and others added 2 commits November 30, 2022 15:21

address linting errors

43199ca

Merge branch 'main' into enable-user-profile-hce-266

0dd7fc5

JolisaBrownHashiCorp merged commit e90102f into main Dec 1, 2022

JolisaBrownHashiCorp deleted the enable-user-profile-hce-266 branch December 1, 2022 16:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable user profile hce 266 #137

Enable user profile hce 266 #137

JolisaBrownHashiCorp commented Nov 23, 2022 •

edited

Loading

JolisaBrownHashiCorp Nov 28, 2022

bcmdarroch Nov 28, 2022

bcmdarroch Nov 28, 2022

bcmdarroch left a comment

bcmdarroch Nov 28, 2022

bcmdarroch Nov 28, 2022

bcmdarroch Nov 28, 2022

bcmdarroch Nov 28, 2022

bcmdarroch Nov 28, 2022

JolisaBrownHashiCorp Nov 29, 2022

bcmdarroch Nov 29, 2022

bcmdarroch Nov 29, 2022

bcmdarroch Nov 29, 2022

bcmdarroch Nov 29, 2022

bcmdarroch Nov 29, 2022

bcmdarroch Nov 29, 2022 •

edited

Loading

bcmdarroch Nov 29, 2022

bcmdarroch Nov 29, 2022

bcmdarroch Nov 29, 2022

bcmdarroch Nov 29, 2022

bcmdarroch Nov 29, 2022

bcmdarroch Nov 29, 2022


		envVarHCPProjectID = "HCP_PROJECT_ID"

		envVarHCPEnvironment = "HCP_ENVIRONMENT"

	profile: profile.UserProfile{},
	profile: &profile.UserProfile{},

	return fmt.Errorf("failed to set profile fields from environment variables (%s, %s): %w", envVarHCPOrganizationID, envVarHCPProjectID, err)
	return fmt.Errorf("failed to configure profile based on environment variables (%s, %s): %w", envVarHCPOrganizationID, envVarHCPProjectID, err)

	// profile is user's the org id, project id, and application environment
	// profile is the user's organization ID and project ID

	// hcpOrganizationID is the user's organization ID.
	// OrganizationID is the user's organization ID.

	// hcpProjectID is the user's project ID.
	// ProjectID is the user's project ID.

	// getOrganizationID() retrieves a user's organization ID.
	// GetOrganizationID() retrieves a user's organization ID.

	// getProjectID() retrieves the user's project ID.
	// GetProjectID() retrieves the user's project ID.

Enable user profile hce 266 #137

Enable user profile hce 266 #137

Conversation

JolisaBrownHashiCorp commented Nov 23, 2022 • edited Loading

🛠️ Description

🔗 External Links

👍 Definition of Done

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bcmdarroch left a 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

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

bcmdarroch Nov 29, 2022 • 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

JolisaBrownHashiCorp commented Nov 23, 2022 •

edited

Loading

bcmdarroch Nov 29, 2022 •

edited

Loading