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

KNOX-3071: New ability in list-alias to list for multiple clusters. N… #940

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

hanicz
Copy link
Contributor

@hanicz hanicz commented Oct 18, 2024

…ew create-list-alias command to create multiple aliases for multiple clusters and also list them.

What changes were proposed in this pull request?

This PR proposes a few changes to the KnoxCLI list-alias and the create-aliases commands. Usage was missing for BatchAliasCreateCommand in printKnoxShellUsage so I added that as well.

With this change the list-alias command will be able to list for multiple clusters. The cluster names have to be concatenated by ','. It is backward compatible with the current usage so it is able to list for a single cluster as well.
Usage: list-alias --cluster1,cluster2,clusterN

The create-aliases command can create multiple aliases for multiple clusters now with a simple call and list them when they are created. An alias will be created for a cluster if it is listed before the cluster. The listing of aliases for the clusters is optional with the use of --list flag. This modification is not backward compatible because previously the --cluster parameter's position didn't matter, however from now on an alias will be created for a cluster if it is listed before the cluster.

Usage: create-aliases --alias alias1 --value value1 --alias alias2 --value value2 --cluster cluster1 --alias alias3 --value value3 --cluster cluster2 --alias aliasN --value valueN --cluster clusterN --list

In the above example alias1 and alias 2 will be created for cluster1, alias3 for cluster2 and aliasN for clusterN. The --value arg is not required as it could be generated or the user will be prompt to provide it. Since --list flag is there the aliases will be listed for the clusters.

How was this patch tested?

I wrote new unit tests to cover the new functionalities and tested manually on my local computer.

@@ -275,6 +277,12 @@ private int init(String[] args) throws IOException {
printKnoxShellUsage();
return -1;
}
} else if (args[i].equals("create-list-aliases")) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this need to be a separate command, or could it be another option to the existing create-alias(es) commands?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It could be the same command. My thinking was that we are introducing two new functionalities which in my opinion could warrant a new command.

  1. With this change we are able to add multiple aliases to multiple topologies.
  2. When the alias creation is done for a specific topology it will list all of the aliases for it.

Copy link
Contributor

Choose a reason for hiding this comment

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

If the new functionality can be added as an option(s) to the existing command without changing the previous behavior of the existing command (without those options), then I think I would prefer this over an entirely separate command. Ultimately, we're just creating aliases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I moved the new functionalities into the existing command. This modification is not backward compatible because previously the --cluster parameter's position didn't matter, however from now on an alias will be created for a cluster if it is listed before the cluster.

…ew create-list-alias command to create multiple aliases for multiple clusters and also list them.
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.

3 participants