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

GEODE-7707: Tab completing '--url' on 'connect' gives two default values #5061

Merged
merged 2 commits into from May 8, 2020

Conversation

alb3rtobr
Copy link
Contributor

this PR aligns the command help and documentation with the code, there is no default value for --url parameter.
Command help will look as follows:

gfsh>connect --url

optional --url: Indicates the base URL to the Manager's HTTP service.  For example: 'http://<host>:<port>/geode-mgmt/v1'; no default value

@alb3rtobr
Copy link
Contributor Author

@moleske as you wrote the ticket, could you review this PR? I think it also needs @davebarnes97 review because documentation is impacted.

Copy link
Contributor

@davebarnes97 davebarnes97 left a comment

Choose a reason for hiding this comment

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

LGTM

@davebarnes97 davebarnes97 requested a review from moleske May 8, 2020 14:40
Copy link
Member

@moleske moleske 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 doing this! The only question I have is more of @davebarnes97 question, but welcome your thoughts also! We can also probably just merge as is if we want

@@ -193,8 +193,8 @@ http://myLocatorHost.example.com:8080/gemfire/v1
"Locator could not find a JMX Manager";
"jmx password must be specified.";
"Could not connect to : {0}. {1}";
"Could not find a GemFire jmx-manager service running at {0}.";
"Could not find a Geode jmx-manager service running at {0}.";
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 have changed lines 196 & 199 to show how these strings really look (they are defined in CliStrings.java)

Copy link
Contributor

Choose a reason for hiding this comment

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

@alb3rtobr @moleske +1 good catch.

@@ -175,16 +175,16 @@ gfsh>connect
``` pre
gfsh>connect
Connecting to Locator at [host=localhost, port=10334] ..
Connecting to Manager at [host=GemFireStymon, port=1099] ..
Successfully connected to: [host=GemFireStymon, port=1099]
Connecting to Manager at [host=GeodeStymon, port=1099] ..
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Lines 178 & 179 are just host names, but as they are the only referece left to "GemFire" I changed it to "Geode"

@moleske
Copy link
Member

moleske commented May 8, 2020

The ApiCheckTestOpenJDK11 is a new job that is failing on all PRs. Merging this is as since there's open JIRA for fixing Api Check

@moleske moleske merged commit 15cb323 into apache:develop May 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants