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

fix(generate): load AWS_PROFILE & AWS_REGION environment variables #1438

Closed
wants to merge 1 commit into from

Conversation

afiune
Copy link
Contributor

@afiune afiune commented Nov 7, 2023

We were not loading AWS environment variables before this change.

How did you test this change?

Wrote integration tests and ran command locally.

Issue

N/A

@afiune afiune requested a review from a team as a code owner November 7, 2023 00:18
Copy link
Contributor

@ipcrm ipcrm left a comment

Choose a reason for hiding this comment

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

I'm wondering if this is always the right thing to do.

I think this is the first place that we’ve deviated from go-sdk controlled settings? I think there’s an interesting question being raised here; e.g. for example if I have AWS_PROFILE set and we're respecting that why aren't we reading the region from that profiles setting? Why not the role (shared config), etc.

I feel like if we want to process these two vars we may need to go a step further and load the SDK so that all host-configurations are picked up. It feels a bit like this is putting ourselves 'half-in' the pool.

Would love to discuss a bit further (or if other reviewers feel this is a non-issue, then I'm happy to be overruled).

@@ -158,6 +158,10 @@ run-api-example: ## Run an API example like 'make run-api-example example=api/_e
LW_SUBACCOUNT=$(shell lacework configure show subaccount) \
go run $(example)

.PHONY: build
build: ## Compiles binary for the running workstation
go build -o bin/lacework -ldflags=$(GO_LDFLAGS) github.com/lacework/go-sdk/cli
Copy link
Contributor

Choose a reason for hiding this comment

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

🎉

@afiune
Copy link
Contributor Author

afiune commented Nov 7, 2023

@ipcrm I appreciate your feedback. Let's hold on this change for now.

@afiune afiune closed this Nov 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants