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

feat: use installation type in cli commands #3675

Merged
merged 1 commit into from
Nov 10, 2023
Merged

feat: use installation type in cli commands #3675

merged 1 commit into from
Nov 10, 2023

Conversation

galibey
Copy link
Contributor

@galibey galibey commented Nov 8, 2023

Installation type (Local, LocalK8, K8, ReadOnly) will be stored in the profile after fluvio cluster start --local/k8/local-k8/read-only and will be used in other cluster operations, so, the user does need to specify it on fluvio cluster delete/shutdown/diagnostics

@galibey galibey changed the title Feat/add installation type to cli feat: use installation type in cli commands Nov 9, 2023
@galibey galibey marked this pull request as ready for review November 9, 2023 10:02
@galibey galibey self-assigned this Nov 9, 2023
@galibey galibey added the CLI label Nov 9, 2023
Copy link
Contributor

@matheus-consoli matheus-consoli left a comment

Choose a reason for hiding this comment

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

LGTM!

#[arg(long)]
no_k8: bool,
}
pub struct ShutdownOpt;
Copy link
Contributor

Choose a reason for hiding this comment

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

Are the any cases where a manual override of the recorded installation_type might be needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There should not be any cases when you install one type and want to override it for delete or shutdown.
But, if there is a need for a hack, it can be done by creating a profile fluvio profile add some_profile localhost:1234 local-k8 with setting another installation type and then execute delete or shutdown.

}
if s.eq_ignore_ascii_case("readonly") {
return Ok(Self::ReadOnly);
}
Copy link
Contributor

@digikata digikata Nov 9, 2023

Choose a reason for hiding this comment

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

Should these different spelling variations be allowed in the installation type parsing from str?

Docs and configs should really only have one spelling to reduce confusion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is involved in the only place where we pass the installation type as an input:

fluvio profile add <PROFILE_NAME> <CLUSTER_ADDRESS> [INSTALLATION_TYPE] 

and while using it I realized that I didn't want to remember if it should be local-k8 or local_k8 etc. So, I added support for all of them.

However, in start command:

fluvio cluster start --local-k8

it is a CLI argument with --, so it only supports one form.

Copy link
Contributor

@digikata digikata left a comment

Choose a reason for hiding this comment

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

LGTM, I added some notes (none intended as blocking).

A lot of the changes were a nice exmple of much cleaner the test commands were in terms of fewer flags as a result of carrying the installaton_type in the profile.

@galibey galibey added this pull request to the merge queue Nov 10, 2023
Merged via the queue into infinyon:master with commit 3d87bf1 Nov 10, 2023
104 checks passed
digikata added a commit that referenced this pull request Nov 10, 2023
This reverts commit 3d87bf1.

CI was unstable when this was merged, need to revert to get back to a stable master
github-merge-queue bot pushed a commit that referenced this pull request Nov 10, 2023
This reverts commit 3d87bf1.

CI was unstable when this was merged, need to revert to get back to a stable master
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