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

Support BROWSER environment variable for overriding how "terraform login" launches a web browser #34902

Open
michaelbrewer opened this issue Mar 28, 2024 · 18 comments · May be fixed by #34904 or #34926
Open
Labels
cloud Related to Terraform Cloud's integration with Terraform enhancement new new issue not yet triaged

Comments

@michaelbrewer
Copy link

michaelbrewer commented Mar 28, 2024

Terraform Version

Terraform v1.7.5
on linux_amd64

Terraform Configuration Files

N/A

Debug Output

N/A

Expected Behavior

Terraform login should use the BROWSER environment variable to find alternatives to open the login window on remote developer environments.

Here is example code of how this is achieved in the coder login cli

browserEnv := os.Getenv("BROWSER")
if browserEnv != "" {
	browserSh := fmt.Sprintf("%s '%s'", browserEnv, urlToOpen)
	cmd := exec.CommandContext(inv.Context(), "sh", "-c", browserSh)
	out, err := cmd.CombinedOutput()
	if err != nil {
		return xerrors.Errorf("failed to run %v (out: %q): %w", cmd.Args, out, err)
	}
	return nil
}

Actual Behavior

Currently terraform login falls back to the lynx browser

Steps to Reproduce

terraform login on a remote environment like gitpod, coder or codespace within a VSCode web or desktop terminal.

Additional Context

NA

References

@michaelbrewer michaelbrewer added bug new new issue not yet triaged labels Mar 28, 2024
@crw crw added cloud Related to Terraform Cloud's integration with Terraform enhancement and removed bug labels Mar 28, 2024
@crw
Copy link
Collaborator

crw commented Mar 28, 2024

Thanks for this feature request! If you are viewing this issue and would like to indicate your interest, please use the 👍 reaction on the issue description to upvote this issue. We also welcome additional use case descriptions. Thanks again!

@michaelbrewer
Copy link
Author

@crw - i have a PR which works

@apparentlymart
Copy link
Member

Hi @michaelbrewer,

Terraform aims to outsource the general problem of launching a browser to the upstream library github.com/pkg/browser, since there are various different conventions on different platforms and so this is a problem better solved in a central location where many codebases can benefit.

Could you try making this feature request upstream first? If the upstream maintainer is willing to include this additional heuristic then I expect we'd be happy to upgrade to a newer version with it implemented, and that would be preferable to having special extra logic in Terraform that may not be appropriate on all platforms where Terraform runs.

If the upstream maintainer is not interested in supporting this then we could consider doing it here in Terraform instead, but I assume they are more familiar with this problem than we are and so I would be interested to hear their reasons for rejecting it so we can decide whether they apply to Terraform too, or whether it's worth us making an exception.

@michaelbrewer
Copy link
Author

It's not a native browser, just that remote development environments would set a standard BROWSER variable to point to shell to run and open the url.

Other tools support this in a similar way like britive cli. Terraform is a cloud enablement tool. So it makes sense to support CDEs and Remote shells and remote development environments.

@michaelbrewer
Copy link
Author

Also note, the golang package has not accepted new contributions for a while. In fact there are a number forks

@michaelbrewer
Copy link
Author

Last human commit is 3 years ago.

image

@apparentlymart
Copy link
Member

apparentlymart commented Apr 1, 2024

I expect we would also be willing to switch to an actively-maintained fork, particularly if it offered equivalent functionality plus the additional environment variable you want.

I do see pkg/browser#41 open in the current library where there was some debate about this idea already, and an earlier PR pkg/browser#14 where it was already declined, so it seems like either way this particular maintainer declined to support this, and some plausible reasons against supporting it, but it seems like the concern was about it not being well-specified yet, which seems like a problem that could be solved if a fork maintainer were sufficiently motivated.

If this were something we were to implement inline in Terraform then I expect we'd choose to use a Terraform-specific environment variable (which conventionally have a TF_ prefix) so that we'd not be contributing yet another slightly-divergent implementation into the mix while this environment variable remains without a well-defined specification. I would prefer to adopt an established standard if possible though, and would prefer that standard to be implemented in a more general place than directly in this codebase.

@michaelbrewer
Copy link
Author

But hack i can tell our fiserv associates is to install a fake xdg-open on linux workspaces to something like

#!/usr/bin/bash
$BROWSER "$1";

But that does not feel right

@michaelbrewer
Copy link
Author

I expect we would also be willing to switch to an actively-maintained fork, particularly if it offered equivalent functionality plus the additional environment variable you want.

I do see pkg/browser#41 open in the current library where there was some debate about this idea already, and an earlier PR pkg/browser#14 where it was already declined, so it seems like either way this particular maintainer declined to support this, and some plausible reasons against supporting it, but it seems like the concern was about it not being well-specified yet, which seems like a problem that could be solved if a fork maintainer were sufficiently motivated.

If this were something we were to implement inline in Terraform then I expect we'd choose to use a Terraform-specific environment variable (which conventionally have a TF_ prefix) so that we'd not be contributing yet another slightly-divergent implementation into the mix while this environment variable remains without a well-defined specification. I would prefer to adopt an established standard if possible though, and would prefer that standard to be implemented in a more general place than directly in this codebase.

Yes as in comment pkg/browser#41 (comment) most people are having to come up with a workaround like the patch i made.

@michaelbrewer
Copy link
Author

michaelbrewer commented Apr 1, 2024

pkg/browser#14 (comment)

If tools need to respect BROWSER in a way they see fit, they can do so manually before invoking pkg/browser functionality.

So either we patch terraform with BROWSER or TF_BROWSER. I would prefer BROWSER as this is what code-server, vscode-server, gitpod, codespaces and codecatalyst - cloud9, openshift devspaces etc..

I mean again we could hack TF_BROWSER to match BROWSER...

TF_BROWSER=$BROWSER

@apparentlymart
Copy link
Member

The main challenge with following the lead of other implementations is that unless we're compatible with what those other implementations expect then we've made things harder rather than easier, because it'll become necessary to reset BROWSER slightly differently depending on whether you're running Terraform or one of those many other examples.

I proposed a TF_ thing only because that neatly avoids us implementing something that is almost-but-not-quite compatible with some other implementation that someone happens to find important but then we can't change it because it's become covered by our compatibility promises. At least if it's a separate environment variable just for Terraform then you have the option of setting it to something slightly different than whatever the other software expects.

If we can figure out some consensus on exactly how BROWSER ought to be interpreted, in a way that's maximally consistent with what's already out there, then that's probably fine, though I'd still prefer to implement it in a separate library that's not specific to Terraform so that at least other software that happens to be written in Go can standardize on the same behavior.

Do you know if any of the other software you care about that uses this environment variable happens to be written in Go? Perhaps we could collaborate with them on extracting their existing implementation into a shared library, if so.


If we do end up with a separate implementation of this (rather than it being encapsulated in the browser library we already depend on) then I think it would be best to implement it as conditionally assigning a different implementation of the existing interface to BrowserLauncher in command.Meta, since that's an existing point we already introduced for switching out different implementations, which we're already using internally for our own mocking, so it seems like a good opportunity to reuse an existing abstraction rather than introducing a new one.

@apparentlymart apparentlymart changed the title Bug: terraform login does not open web browser Support BROWSER environment variable for overriding how "terraform login" launches a web browser Apr 1, 2024
michaelbrewer added a commit to michaelbrewer/terraform that referenced this issue Apr 2, 2024
@michaelbrewer
Copy link
Author

@apparentlymart is this what you mean ?

#34926

Which added a flag to use the browser environment variable to launch the browser (TF_BROWSER_ENV)

@michaelbrewer
Copy link
Author

@apparentlymart i have to PRs with different implementations to choose from

@crw
Copy link
Collaborator

crw commented Apr 5, 2024

@michaelbrewer Just FYI we will be discussing this in triage on Monday. Thanks!

@michaelbrewer
Copy link
Author

@crw any updates? hopefully the use case makes sense.

@crw
Copy link
Collaborator

crw commented Apr 12, 2024

Yes, this is to be reviewed. I think this week got a little busy with the 1.8 GA release, but I've put it back on the radar.

@michaelbrewer
Copy link
Author

@crw any updates?

@crw
Copy link
Collaborator

crw commented Apr 25, 2024

No, I bumped it again. I may need to get this re-prioritized. Thanks for checking in.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cloud Related to Terraform Cloud's integration with Terraform enhancement new new issue not yet triaged
Projects
None yet
3 participants