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

Change setupEnv to return an ExpressionEvaluator #423

Merged
merged 1 commit into from
Jan 13, 2021

Conversation

winksaville
Copy link
Contributor

This is a solution to issue #416 where environment variables created or
changed in the previous step are not usable in the next step because
the rc.ExprEval is from the beginning of the previous step.

This change refactors setupEnv so that before interpolating the environment
variables a NewExpressionEvaluator is created.

Fixes: 416

@winksaville
Copy link
Contributor Author

@cplee here is a proposed solution to issue #416, I need to create at least one test. Could you advise me on the best way to create a test where environment variables are created and or altered in a step and then the next step uses the new or altered enironment variables?

@winksaville
Copy link
Contributor Author

@torbjornvatn, I'd appreciate any suggestions you might have too.

@torbjornvatn
Copy link
Contributor

@winksaville Good catch! I think this looks like a good solution, but some tests would have been nice.
run_context_test.go can be used for inspiration for how to set one up.

@winksaville
Copy link
Contributor Author

..., but some tests would have been nice.

I agree, tests are needed. I looked at run_context_text.go, but I don't see how to create a test with multiple steps. I'm going to see if I can create a simpler test then my act-failing-python-cache t1.yml in .github/workflows/ and then hopefully convert that to a go test.

Is there currently a "go test" that I can use as a guide that runs multiple steps?

winksaville added a commit to winksaville/act that referenced this pull request Nov 17, 2020
Test issue nektos#416. With PR nektos#423 the test passes and fails otherwise.

To facilitate this new test I refactored RunTestJobFile out of
TestRunEvent and use it in TestRunEvent and TestPythonCache
@winksaville
Copy link
Contributor Author

I've added a test, 88ba816, this is using an docker image I made and is more complex than we actually need, but it does pass with this change and fails without it, so I feel that I can actually make some kind of test.

Please let me know if this test style is deseriable and if my adding of RunTestJobFile is OK. If so I'd like to create a couple of simple actions that just change and create some environemt variables without the need to actually install python or use my docker image and can instead just use the node:12.6-buster-slim image.

@torbjornvatn
Copy link
Contributor

I think this looks great.
I suggest that you un-draft this PR and let @cplee have a look at it

winksaville added a commit to winksaville/act that referenced this pull request Nov 18, 2020
Test issue nektos#416. With PR nektos#423 the test passes and fails otherwise.

To facilitate this new test I refactored RunTestJobFile out of
TestRunEvent and use it in TestRunEvent and TestPythonCache
@winksaville winksaville marked this pull request as ready for review November 18, 2020 17:56
@winksaville
Copy link
Contributor Author

@cplee I've marked this ready for review as suggested by @torbjornvatn and I'd like you to check if this is going in a reasonable direction. As I said here I'm looking into how to make a simpler test which doesn't need to download the docker image I created.

@winksaville
Copy link
Contributor Author

When #426 is merged ./actions/docker-local-set-env-var/entrypoint.sh should be changed to use $GITHUB_ENV.

@cplee
Copy link
Contributor

cplee commented Nov 30, 2020

@winksaville #429 has been merged.

Why is this PR so large? +88,120 😱

@winksaville
Copy link
Contributor Author

Why is this PR so large? +88,120

It's so largre because actions/js-local-set-env-var contians all of the required node/npm dependencies.

Instead of making it a "local" action it could be made "remote" and saved in github.com/nektos/gha-js-set-env-var, like github.com/winksaville/gha-js-set-env-var.

@winksaville
Copy link
Contributor Author

@cplee anything more you'd like me to do so this can be merged?

@cplee
Copy link
Contributor

cplee commented Dec 8, 2020

@winksaville - i'm not comfortable with this PR given the fact there is 88,062 additions 😱

Can you just stick with the package.json and run npm install instead of committing all the deps?

@winksaville
Copy link
Contributor Author

@winksaville - i'm not comfortable with this PR given the fact there is 88,062 additions

Can you just stick with the package.json and run npm install instead of committing all the deps?

Github seems to suggest it's not a good idea, see this. They do offer another suggestion in that section and say:

"As an alternative to checking in your node_modules directory you can use a tool called @vercel/ncc to compile your code and modules into one file used for distribution."

Although I suspect that will still be a large addition by line count.

Personally, I'd lean towards you adding js-set-env-var as a "remote" action checked into github.com/nektos, as I suggested above.

But your call, let me know what you'd like me to do.

This is a solution to issue nektos#416 where environment variables created or
changed in the previous step are not usable in the next step because
the rc.ExprEval is from the beginning of the previous step.

This change refactors setupEnv so that before interpolating the environment
variables a NewExpressionEvaluator is created.


Fixes: 416
@cplee cplee merged commit f2c1507 into nektos:master Jan 13, 2021
@winksaville winksaville deleted the fix-issue-416 branch January 13, 2021 00:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants