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

Helm values files for CLI #1974

Merged
merged 1 commit into from
Sep 13, 2023

Conversation

scottboring
Copy link
Contributor

@scottboring scottboring commented Aug 28, 2023

Description

Adding optional flag for values file similar to helm values file making CLI easier to use when multiple values need to be overridden. The new flag is --values, the same as helm and functions the same way in that is supports multiple files including files from URLs.

How Has This Been Tested?

Tested multiple scenarios locally

  • Install help message
  • Install without values param
  • Install with 1 values param
  • Install with 2 values params
  • Install with --set overriding --values params
  • Install fails with invalid values file
  • Install fails values file not found

@adamjensenbot
Copy link
Collaborator

Hi @scottboring. Thanks for your PR!

I am @adamjensenbot.
You can interact with me issuing a slash command in the first line of a comment.
Currently, I understand the following commands:

  • /rebase: Rebase this PR onto the master branch (You can add the option test=true to launch the tests
    when the rebase operation is completed)
  • /merge: Merge this PR into the master branch
  • /build Build Liqo components
  • /test Launch the E2E and Unit tests
  • /hold, /unhold Add/remove the hold label to prevent merging with /merge

Make sure this PR appears in the liqo changelog, adding one of the following labels:

  • kind/breaking: 💥 Breaking Change
  • kind/feature: 🚀 New Feature
  • kind/bug: 🐛 Bug Fix
  • kind/cleanup: 🧹 Code Refactoring
  • kind/docs: 📝 Documentation

@cheina97 cheina97 self-requested a review August 29, 2023 06:54
@cheina97 cheina97 self-assigned this Aug 29, 2023
@cheina97
Copy link
Member

cheina97 commented Aug 29, 2023

Hi @scottboring thanks for your PR, that's something we planned to implement and I'm sure it will be really useful in the future. So thanks for your commitment to our project and your time, we really appreciate it.

Just a few comments:

  1. I think it would be more correct to implement the behaviour of the --values flag here. You can override the values file retrieved by helm.
  2. Suggestion: get inspiration from here in order to emulate the helm --values flag behaviour
  3. I would expect the --values flag does not have priority on the rest of the flags, especially --set (like in helm).
  4. Please adds an explanation of the --values flag in the documentation.
  5. Please put the PR in draft till it is ready (it includes linting check).

@scottboring scottboring marked this pull request as draft August 29, 2023 20:42
@scottboring
Copy link
Contributor Author

Hi @cheina97, thank you for the review and comments.

  1. I don't follow this, how would this logic be implemented here and why wouldn't it be next to the logic for the --set flag?
  2. I can make some changes to use or copy that code
  3. The --values flag overrides is ran before the --set flag so this priority is aligns with your expectations. I have also tested this.
  4. I'lll update the documentation
  5. This has been converted to draft

If you could please provide some more detail around the points 1 & 2, that would be helpful to ensure the changes I make align with your expectations.

@cheina97
Copy link
Member

cheina97 commented Aug 29, 2023

1/3. You are right, it's better to place the --values override where it is right now.
2. I think it's a good idea to copy that code to be sure to behave like expected in Helm.

@scottboring
Copy link
Contributor Author

Hi @cheina97, I've made the requested updates. Thank you for reviewing my PR and providing feedback. Let me know if there is any other feedback.

Note that I used the Helm cli env settings here which I'm not sure is correct but aligns with how Helm works: https://github.com/liqotech/liqo/pull/1974/files#diff-a849a3ee27a1eaa15291b34f78d4adee1ef1e4e268d48eb3dd1338acb2f2c685R422

docs/installation/install.md Outdated Show resolved Hide resolved
pkg/liqoctl/install/handler.go Show resolved Hide resolved
@cheina97
Copy link
Member

cheina97 commented Sep 6, 2023

Please, launch make generate to fix the majority of lining errors.

@scottboring scottboring marked this pull request as ready for review September 12, 2023 13:36
@scottboring
Copy link
Contributor Author

Hi @cheina97, I believe this PR is ready to be merged.

@cheina97
Copy link
Member

Please, can you squash the commits?

@scottboring
Copy link
Contributor Author

@cheina97 commits have been squashed

@cheina97
Copy link
Member

/rebase test=true

…ng CLI easier to use when multiple values need to be overridden
Copy link
Member

@cheina97 cheina97 left a comment

Choose a reason for hiding this comment

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

lgtm

@cheina97
Copy link
Member

/merge

@adamjensenbot adamjensenbot added the merge-requested Request bot merging (automatically managed) label Sep 13, 2023
@adamjensenbot adamjensenbot merged commit a632fee into liqotech:master Sep 13, 2023
@adamjensenbot adamjensenbot removed the merge-requested Request bot merging (automatically managed) label Sep 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants