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

Add micromamba deinit in post.js #89

Merged
merged 8 commits into from Aug 30, 2022
Merged

Conversation

pavelzw
Copy link
Member

@pavelzw pavelzw commented Aug 17, 2022

Adds the micromamba deinitialization in the post-action.
Closes #79.

Tested here.

@pavelzw pavelzw force-pushed the deinit branch 2 times, most recently from c7d0f1a to 064a0cf Compare August 20, 2022 14:32
Copy link
Collaborator

@jonashaag jonashaag left a comment

Choose a reason for hiding this comment

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

GitHub broke my review

action.yml Outdated Show resolved Hide resolved
Copy link
Collaborator

@jonashaag jonashaag left a comment

Choose a reason for hiding this comment

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

⚠️ Note that using getInput is NOT safe in a post action because if the calling workflow uses templated inputs, they will generally evaluate to something different in the post action:

- using: foo
  with:
    # Using 'x' in post is not safe
    x: ${{ some_templated_var }}

"Official workaround": https://github.com/actions/cache/blob/a7c34adf76222e77931dedbf4a45b2e4648ced19/src/restore.ts#L24-L25

Feel free to double check if this has changed in the past months

post.js Outdated Show resolved Hide resolved
post.js Outdated Show resolved Hide resolved
@jonashaag
Copy link
Collaborator

jonashaag commented Aug 20, 2022

Note that NONE of the INPUTS.xyz will generally work as expected in the post action because they might be templated. You can try to just JSON-dump the entire INPUTS to the state.

@pavelzw
Copy link
Member Author

pavelzw commented Aug 20, 2022

@jonashaag

You can try to just JSON-dump the entire INPUTS to the state.

Done.

Copy link
Collaborator

@jonashaag jonashaag left a comment

Choose a reason for hiding this comment

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

Looks good!

Let's add a test (would've caught the bug below!)

action.yml Outdated Show resolved Hide resolved
util.js Outdated Show resolved Hide resolved
@jonashaag
Copy link
Collaborator

Unrelated to this PR, I just saw that we have our own implementation of rmRf but we could also use io.rmRF.

pavelzw and others added 2 commits August 29, 2022 20:34
Co-authored-by: Jonas Haag <jonas@lophus.org>
@pavelzw
Copy link
Member Author

pavelzw commented Aug 29, 2022

Let's add a test (would've caught the bug below!)

Checking the state after the post action is not easy. You need to execute another action before p-w-m that executes your commands in their post action. Luckily, there is an action for that: webiny/action-post-run. Unfortunately, this action does not pipe the errors out and thus won't fail if an error occurs. This is fixed in the fork lisanna-dettwyler/action-post-run.
If webiny/action-post-run updates its action, we can use it. For now, let's stick with lisanna-dettwyler/action-post-run.

.github/workflows/test_deinit.yml Show resolved Hide resolved
.github/workflows/test_deinit.yml Show resolved Hide resolved
.github/workflows/test_deinit.yml Show resolved Hide resolved
Co-authored-by: Jonas Haag <jonas@lophus.org>
@jonashaag jonashaag merged commit b54d7bb into mamba-org:main Aug 30, 2022
jonashaag added a commit that referenced this pull request Aug 30, 2022
@jonashaag
Copy link
Collaborator

Whoops, early merge, let’s hope CI is good

@pavelzw pavelzw deleted the deinit branch August 31, 2022 06:25
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.

Add option not to change any user files (or revert changes)
2 participants