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

Set env variables on Gitpod per terminal #3573

Merged
merged 8 commits into from
Oct 25, 2021
Merged

Set env variables on Gitpod per terminal #3573

merged 8 commits into from
Oct 25, 2021

Conversation

mikenikles
Copy link
Contributor

This is a follow-up to a discussion on Discord (message link).

The before start task makes sure the env variables are available for the init and command tasks in both terminals.

Please give that a go for a while and do let me know if you still see flaky behaviour with env vars.

@jtoar
Copy link
Contributor

jtoar commented Oct 15, 2021

Hey @mikenikles thanks for the fix; we've gone back and forth a few times on this and one question I have is: is there a way to set a repo-level env var, that's not specific to a terminal? That's what we were trying to do with the gp cli:

eval $(gp env -e RWFW_PATH="/workspace/redwood")
eval $(gp env -e RWJS_DEV_API_URL="http://localhost")

@thedavidprice
Copy link
Contributor

@jtoar but is it the repo-level that's the context or is it the GitPod container/workspace? I might not understand correctly, but based on the change here, GitPod will now include the env vars in the workspace (container?) environment, which is where we wanted it to be all along.

@thedavidprice
Copy link
Contributor

Confirming this is working well in GitPod workspace. I tested here:
https://gold-lynx-yfauo4fk.ws-us17.gitpod.io/

@thedavidprice thedavidprice self-assigned this Oct 16, 2021
@thedavidprice thedavidprice self-requested a review October 16, 2021 20:46
@thedavidprice thedavidprice added this to the next-release-priority milestone Oct 16, 2021
@jtoar
Copy link
Contributor

jtoar commented Oct 16, 2021

@thedavidprice I want what you're describing but this PR as I understand it only sets it for each terminal (which is why there's two before steps, one for each), not for the whole workspace/container/etc. So it goes away when the terminal's exited.

To be clear this is still a valid solution that I'm happy with, it just means that if a user exits both terminals (for whatever reason) before running yarn rwfw project:sync, the env var won't be persisted via configstore

@thedavidprice
Copy link
Contributor

So it goes away when the terminal's exited.

AH, got it. Yeah, that's not ideal. (And not what I understood.)

@jtoar
Copy link
Contributor

jtoar commented Oct 19, 2021

@thedavidprice here's the gitpod issue I'm using to track the status of the project-level environment variables: gitpod-io/gitpod#4456.

Seeing as they're not a feature yet, and that we've changed this config quite a few times already without lasting success, I'm all for just merging this as the solution for now and dealing with (via docs) the user-exits-both-terminals edge case.

@jtoar jtoar self-assigned this Oct 19, 2021
@thedavidprice
Copy link
Contributor

Seeing as they're not a feature yet, and that we've changed this config quite a few times already without lasting success, I'm all for just merging this as the solution for now and dealing with (via docs) the user-exits-both-terminals edge case.

Personally, I have not experienced strange behavior with the current implementation. Also, based on the Issue you linked I'm not expected project-level env vars any time soon.

I think for me to say, "Ok, let's do it", I'd want us to first have a visible message and/or docs about this behavior. I.e. if you close both terminals, both yarn rwfw and yarn rw dev will no longer appear to work.

@thedavidprice
Copy link
Contributor

Q: is there a way we can have a message that displays in a terminal whenever it's started?

@jtoar
Copy link
Contributor

jtoar commented Oct 24, 2021

Personally, I have not experienced strange behavior with the current implementation.

@thedavidprice Here's a screenshot showing what I'm seeing, that the env var RWFW_PATH isn't defined (bottom left terminal). I'm just using our repo with out current config, via gitpod.io/#https://github.com/redwoodjs/redwood:

image

I've tried this PR and it works; I see it defined there.

Q: is there a way we can have a message that displays in a terminal whenever it's started?

We do have a message that displays actually but we can add to it for sure.

Copy link
Contributor

@dac09 dac09 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 going to merge this in, because atleast it will be more consistent the gp env method. With the caveat that opening a new terminal window will mean it won't have these vars defined, only the default ones will.

@mikenikles - just something to be aware of, I think there maybe a bug in the gp cli, where it doesn't always set env variables this way.

The challenge is the person writing the original PR never sees this, only other people trying to use it sometimes find that the env var isn't defined. I suspect that's because gp env applies on a per account basis, and something goes wrong there when the PR is in a forked repo

@thedavidprice
Copy link
Contributor

We do have a message that displays actually but we can add to it for sure.

@dac09 Can we at least do something like ^^ to give some info about terminal behavior re: Env Vars?

Copy link
Contributor

dac09 commented Oct 25, 2021

@dac09 Can we at least do something like ^^ to give some info about terminal behavior re: Env Vars?

Done ✅

Copy link
Contributor

@thedavidprice thedavidprice left a comment

Choose a reason for hiding this comment

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

ship it 🚀

@dac09 dac09 merged commit 1455876 into redwoodjs:main Oct 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants