Skip to content

Fix/volume prompt #267

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

Merged
merged 3 commits into from
Sep 9, 2024
Merged

Fix/volume prompt #267

merged 3 commits into from
Sep 9, 2024

Conversation

lgaroche
Copy link
Contributor

@lgaroche lgaroche commented Sep 6, 2024

Skip prompts for program volumes when at least one volume is passed in command line parameters.
It caused a but where the parameters are only taken into account if all three are passed.

Fixes: #230

Ideally: the --non-interactive option from #197 would provide a more predictable behaviour.

Self proofreading checklist

  • The new code clear, easy to read and well commented.
  • New code does not duplicate the functions of builtin or popular libraries.
  • An LLM was used to review the new code and look for simplifications.
  • New classes and functions contain docstrings explaining what they provide.
  • All new code is covered by relevant tests.

Changes

If one of --immutable-volume, --persistent-volume or --ephemeral-volume is passed as a parameter, skip the volume prompt and take these inputs instead.

How to test

Example:
aleph program upload app main:app --immutable-volume ref=$VOLUME_ITEM_HASH,mount=/opt/packages

Copy link
Member

@hoh hoh left a comment

Choose a reason for hiding this comment

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

Thanks @lgaroche for this Pull Request !

@philogicae , can you review this branch and look at my suggested simplifications as well ?

@hoh hoh assigned hoh and philogicae Sep 9, 2024
@hoh hoh requested a review from philogicae September 9, 2024 09:53
Copy link
Member

@philogicae philogicae left a comment

Choose a reason for hiding this comment

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

LGTM

@philogicae philogicae merged commit 2dba0a0 into aleph-im:master Sep 9, 2024
8 checks passed
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.

Attaching arguments to the command
3 participants