-
Notifications
You must be signed in to change notification settings - Fork 10
Ensuring env var is always taken over account backend url #477
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
Conversation
5da6ac6
to
45ac8b5
Compare
@IaroslavTitov I don't agree with your statement here:
I even have a complete opposite standpoint. If someone is working with multiple accounts, using different values for a specific environment variable allows for easy switching, definitely if you do this using direnv. This all becomes automatic by going into a specific directory. I would also favor to remain consistent with the core Pulumi CLI, where I requested this via pulumi/pulumi#13919 and this was integrated in Pulumi v3.130.0. |
cmd/esc/cli/login.go
Outdated
if backendURL != account.BackendURL { | ||
account, err = esc.workspace.GetAccount(backendURL) | ||
if err != nil { | ||
return fmt.Errorf("could not determine current cloud: %w", err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not correct IMO. If both PULUMI_BACKEND_URL
and PULUMI_ACCESS_TOKEN
are defined, and this is the first time that we use this backend, Pulumi will auto login using these environment variables.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would also favor to remain consistent with the core Pulumi CLI
Sounds good, I didn't realize there's a precedent with Pulumi CLI, ignore the comment 😅
This is not correct IMO
I was trying to prevent user footgun, when one sets PULUMI_BACKEND_URL
, but not the token. In that case, Error: listing environments: [401] Unauthorized: No credentials provided or are invalid.
will be returned. So the scenario in the ticket wouldn't work unless a token is specified along with the backend URL. But yeah, if that's the desirable behavior, I'll just remove this block.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, changed the check to run login if backendURL is different than the one in logged in account - this way it runs the same login logic, preferring PULUMI_ACCESS_TOKEN
, but still falling back to accounts, so best of both worlds
Our use case here (besides consistency with the pulumi cli): Suppose you have two shell sessions open on different stacks using different backends. Using the env var will make running the two possible in a seamless way (e.g. by using direnv) where a |
5a359c9
to
398c9b5
Compare
Can we add tests for this? |
398c9b5
to
ff24a38
Compare
ff24a38
to
8073e9d
Compare
8073e9d
to
d9af007
Compare
Done! Please take a look, recreated the scenario from the issue as a test |
This PR has been shipped in release v0.13.1. |
This PR has been shipped in release v0.13.2. |
Summary
pulumi env ls
doesn't respectPULUMI_BACKEND_URL
#473PULUMI_BACKEND_URL
over backend url saved into the accountPULUMI_ACCESS_TOKEN
env var, falling back to accounts if it's not specifiedTesting
Error: no credentials. Please run
esc loginto log in.