Open michaelbrewer opened 6 months ago
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!
@crw - i have a PR which works
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.
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.
Also note, the golang package has not accepted new contributions for a while. In fact there are a number forks
Last human commit is 3 years ago.
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 https://github.com/pkg/browser/issues/41 open in the current library where there was some debate about this idea already, and an earlier PR https://github.com/pkg/browser/pull/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.
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
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 https://github.com/pkg/browser/issues/41#issuecomment-1277079745 most people are having to come up with a workaround like the patch i made.
https://github.com/pkg/browser/pull/14#issuecomment-741864029
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
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 is this what you mean ?
https://github.com/hashicorp/terraform/pull/34926
Which added a flag to use the browser environment variable to launch the browser (TF_BROWSER_ENV
)
@apparentlymart i have to PRs with different implementations to choose from
@michaelbrewer Just FYI we will be discussing this in triage on Monday. Thanks!
@crw any updates? hopefully the use case makes sense.
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.
@crw any updates?
No, I bumped it again. I may need to get this re-prioritized. Thanks for checking in.
@crw any word? Fiserv (like any regulated enterprise company) no longer allows software like terraform to be installed on the corporate endpoint, so getting terraform cli working on a cloud development environment is becoming critical
@crw i have updated both pull requests #34904 and #34926
The engineer who had committed to review it left the team, and it fell through the cracks. I'll try to find a new owner.
Terraform Version
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
cliActual 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
BROWSER
env only)TF_BROWSER_ENV
env flag)