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 an option for install directory of mini{forge,conda} #342

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

isuruf
Copy link
Member

@isuruf isuruf commented Apr 11, 2024

This is useful for self-hosted runners where we can change the installation directory to the workspace directory to allow automatic cleaning up of the miniforge/miniconda environments.

@isuruf isuruf requested a review from a team as a code owner April 11, 2024 20:13
@isuruf
Copy link
Member Author

isuruf commented Apr 15, 2024

cc @finnagin

Copy link
Member

@jaimergp jaimergp left a comment

Choose a reason for hiding this comment

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

Just one comment about the name chosen for now. Implementation looks ok. We should add an example test to make sure this works as intended.

@@ -29,6 +29,11 @@ inputs:
installers"
required: false
default: ""
install-dir:
Copy link
Member

Choose a reason for hiding this comment

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

I'm inclined to using installation-directory here instead. The rest of the options in this project seem to be using no abbreviations (e.g. we fully spell environment instead of env).

Copy link
Member

Choose a reason for hiding this comment

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

Hi @isuruf, indeed I agree with @jaimergp comment, we should try to use full names :)

Copy link
Member

@goanpeca goanpeca left a comment

Choose a reason for hiding this comment

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

Thanks for working on this @isuruf

Could we add information on this usage on the README?

Also could we add some sort of workflow to test this?

If this only makes sense for self hosted runners, maybe we could add something on this PR #301 ?

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.

None yet

3 participants