HPCC-18038 Remove the version option from the publish and java command #10316

mayx · 2017-07-31T18:46:32Z

Remove the --version option from the usage message
Don't pass in any version info when loading dependencies,
pass in 0 instead.
Don't validate version command option
Replace --version with --build for printing out build info
Remove the --version option from the batch file for ECL IDE

Signed-off-by: mayx yanrui.ma@lexisnexisrisk.com

Type of change:

This change is a bug fix (non-breaking change which fixes an issue).
This change is a new feature (non-breaking change which adds functionality).
This change improves the code (refactor or other change that does not change the functionality)
This change fixes warnings (the fix does not alter the functionality or the generated code)
This change is a breaking change (fix or feature that will cause existing behavior to change).
This change alters the query API (existing queries will have to be recompiled)

Checklist:

Testing:

hpcc-jirabot · 2017-07-31T18:46:54Z

https://track.hpccsystems.com/browse/HPCC-18038
Jira updated

mayx · 2017-07-31T18:52:12Z

@rpastrana @afishbeck Please review when you have a chance.

rpastrana · 2017-07-31T19:58:41Z

tools/esdlcmd/esdlcmd_shell.cpp

@@ -180,7 +180,7 @@ bool EsdlCMDShell::parseCommandLineOptions(ArgvIterator &iter)
 void EsdlCMDShell::usage()
 {
    fprintf(stdout,"\nUsage:\n"
-        "    esdl [--version] <command> [<args>]\n\n"


We should provide usage for the new "build" command, but also consider keeping this version command to remain consistent across various HPCC command line tools (ecl, eclcc, etc).

I did add usage for --build in the common command usage. What I'm trying to do is to avoid overloading the meaning of --version, as although we're removing it from publish and java, it's still being used for specifying esdl version in xsd and wsdl commands.

I could change the xsd and wsdl to use --ver_ (which nicely corresponds to the url parameter name to specify service version), and restore --version solely to printout build info. Do you guys like that better?

makes sense, maybe the tool version command remains --version and the interface version gets an explicit name --interfaceversion or some shorter derivation of it?

Agreed that we should use --version the way other commands do (like ecl.exe) rather than --build.

Where we do want interface version specified, the long intuitive name should be something like "--interface-version".. for people who haven't memorized the short easy name, something like "-iv", or if you guys have a better idea for the short name I'm open.

"--" should always be a long form very intuitive and explicit name. So not --ver_. Single "-" should be a short name, often an abbreviation or single letter. But again I think -ver_ is a bit too awkward.

Sounds reasonable, I'll replace --version with --interface-version and -iv, then replace --build with --version.

mayx · 2017-08-01T21:24:28Z

Changes made as suggested. @rpastrana @afishbeck please review again.

rpastrana

@mayx A couple of concerns, I might have clouded the issue with my attempt to explain my concerns... don't hesitate to contact me directly if my explanation doesn't make sense.

rpastrana · 2017-08-02T14:33:15Z

tools/esdlcmd/esdl-publish.cpp

@@ -79,7 +79,6 @@ class EsdlPublishCmdCommon : public EsdlCmdCommon
                "   --port <port>                WsESDLConfig service port\n"
                "   -u, --username <name>        Username for accessing WsESDLConfig service\n"
                "   -pw, --password <pw>         Password for accessing WsESDLConfig service\n"
-                "   --version <ver>              ESDL service version\n"


I think we need to keep this line in the base class usage (and update it to "--interface-version, ESDL interface version\n") since all extensions of this class support should support --version (now --interface-version)
If publish command won't acknowledge this argument, then that class needs to override this usage.

Actually the --version (now --interface-version) is not being used by other publish commands. A couple of other commands takes a "definition version/sequence#" as parameter but that is specified on the command line directly, not with --version (now --interface-version) option. I don't think any of the publish commands need or should specify the interface version.

ahh.. ok so then StringAttr optVersionStr; and double optVersion; are actually being used as "esdl_definition_version"?? maybe we refactor them to esdl_def_ver?

rpastrana · 2017-08-02T14:39:43Z

tools/esdlcmd/esdl-publish.cpp

@@ -94,8 +93,6 @@ class EsdlPublishCmdCommon : public EsdlCmdCommon
            return true;
        if (iter.matchFlag(optWSProcPort, ESDL_OPTION_SERVICE_PORT) || iter.matchFlag(optWSProcPort, ESDL_OPT_SERVICE_PORT))
            return true;
-        if (iter.matchFlag(optVersionStr, ESDLOPT_VERSION))


This if needs to stay, and check for ESDLOPT_INTERFACE_VERSION || ESDLOPT_IV... we might also want to refactor optVersionStr and optVersion to reflect their actual purpose which is to hold the InterfaceVersion value as declared by the user.

--UPDATE-- looking through the code, I just realized that this member var is also used to hold the ESDL definition version for EsdlDeleteESDLDefCmd; EsdlBindMethodCmd. I don't like overloading variables, so we can either declare a new member var for those classes to hold the ESDL definition version, or live with the way things are, but then we can't refactor optVersionStr and optVersion as suggested above

Yes this is another thing I was thinking to change, but thought it would tie better with another improvement subtask I may make. But maybe changing the variable name now will make it more clear, let me check...

Also, while we're at it...
It seems the usage at the top level might be off:
esdlcmd_shell.cpp:
esdl [--version] []\n\n"
should change to
esdl []\n\n"
and --version should be listed as a command

@JamesDeFabia this change might require a doc change (I think an issue is already open, but wanted to point this out)

I may also refactor the variable name for the definition sequence. What better describes this variable, "definitionVersion" or "defintionSequence", @rpastrana @afishbeck ?

both are appropriate, sequence might be more accurate, but I think we refer to that number as the definition version in other places. I vote definitionVersion

I agree with moving --version as a command.

mayx · 2017-08-02T22:02:31Z

Did the variable name refactoring, also added a separate line for --version usage. @rpastrana @afishbeck please review again.

afishbeck · 2017-08-02T23:56:47Z

tools/esdlcmd/esdl-publish.cpp

@@ -615,7 +599,7 @@ class EsdlDeleteESDLDefCmd : public EsdlPublishCmdCommon
                    optESDLService.set(arg);
                    break;
                case 1:
-                    optVersionStr.set(arg);
+                    optDefinitionVersionStr.set(arg);


It seems inconsistent to me that when we bind a service we use a definition ID of the format "MyDefinition.3", but here we use a separate ESDLService and Version "MyDefinition 3".

I prefer the ID type usage... which is how roxie queries work... "myquery.5".

Especially when it looks like we turn it into an ID before sending it to ESP.

If we make that change we may not need the variable anymore, but I also wonder if the name "PublishedVersion" is more descriptive... meaning the particular version of the ESDLService that was published for "myserv.3"

Agreed, my original implementation introduced this inconsistency. Changing things now will break users, but we might have to tear off the band-aid quickly and move on.
If we make the change, we could make the fix backward compatible, but I'm not sure if that would be worth the effort.

Sorry missed this string of comments. I was thinking to deal with this inconsistency in a separate issue, but since we're already here and touched on the code already, I may just do it now.

afishbeck · 2017-08-03T00:08:56Z

tools/esdlcmd/esdlcmd_common.hpp

@@ -62,6 +62,8 @@ typedef IEsdlCommand *(*EsdlCommandFactory)(const char *cmdname);
 #define ESDLOPT_XSLT_PATH               "--xslt"

 #define ESDLOPT_VERSION                 "--version"
+#define ESDLOPT_INTERFACE_VERSION       "--interface-version"
+#define ESDLOPT_IV                      "-iv"


"-iv" is considered an abbreviated or short form of "--interface-version" so there is a naming convention to just add an "_S". Making it #define ESDLOPT_INTERFACE_VERSION_S "-iv"

Like this example:

#define ECLOPT_INPUT "--input"
#define ECLOPT_INPUT_S "-in"

afishbeck

A few comments

mayx · 2017-08-03T14:28:55Z

Changed ESDLOPT_IV to ESDLOPT_INTERFACE_VERSION_S, please review again. There're a few other existing options that don't follow this convention, but I think it would be better to address them in a separate refactoring issue.

- For publish and java commands, remove the version option and don't pass in version when loading dependencies - Don't validate version command option for publish and java - Remove the --version option from the batch file for ECL IDE - Remove the --version from ReadMeFirst.txt help file - Refactor the variable name optVersion to optInterfaceVersion for xsd and wsdl commands - Change defintion version to definition id for commands delete and bind-method. Signed-off-by: mayx <yanrui.ma@lexisnexisrisk.com>

mayx · 2017-08-07T15:45:27Z

Removed optDefinitionVersion variable, changed definition version to definition id for commands delete and bind-method. @afishbeck @rpastrana please review again.

HPCCSmoketest · 2017-08-07T16:09:26Z

Automated Smoketest: ✅
Sha: 7a0d3de
Build: success
Build: success
ECL Watch: Rebuilding Site

errors	warnings	build time
0	65	91.65 seconds

Install hpccsystems-platform-community_6.4.1-closedown0.el7.x86_64.rpm
HPCC Start: OK

Unit tests result:

Test	total	passed
unittest	95	95
wutoolTest(Dali)	19	19
wutoolTest(Cassandra)	19	19

Regression test result:

phase	total	pass
setup (hthor)	11	11
setup (thor)	11	11
setup (roxie)	11	11
test (hthor)	733	733
test (thor)	627	627
test (roxie)	760	760

HPCC Stop: OK
HPCC Uninstall: OK

afishbeck · 2017-08-07T17:29:18Z

tools/esdlcmd/esdl-publish.cpp

@@ -863,7 +826,7 @@ class EsdlBindMethodCmd : public EsdlBindServiceCmd
        printf( "\n Use this command to publish ESDL Service based bindings.\n"
                "   To bind a ESDL Service, provide the target ESP process name\n"
                "   (esp which will host the service.) \n"
-                "   It is also necessary to provide the Port on which this service is configured to run (ESP Binding),\n"
+                "   It is also necessary to provide the target ESP binding name,\n"


Does just specifying the port not work? I think using port is much more intuitive than ESP binding name... which is usually a confusing unique id. If it doesn't work, please open a JIRA to add it (to any command line that currently requires ESP binding name). If it does work the help should still say so (in fact it should be considered the preferred way.

@mayx @rpastrana Btw, in the future when this is fully dynamic the port will still make sense, but the ESP binding name will no longer make sense.

For this command (bind-method), it doesn't take port as a parameter. I've created an issue for it: https://track.hpccsystems.com/browse/HPCC-18126

afishbeck

@mayx some comments

afishbeck

@mayx looks good

rpastrana · 2017-08-08T14:20:54Z

tools/esdlcmd/esdl-publish.cpp

-                "   ESDLServiceDefinitionName         The name of the ESDL service definition to delete\n"
-                "   ESDLServiceDefinitionVersion      The version of the ESDL service definition to delete\n");
+                "esdl delete <ESDLDefinitionID> [command options]\n\n"
+                "   ESDLDefinitionID      The ESDL definition id <esdldefname>.<esdldefver>\n");


@JamesDeFabia note this change. It needs to be documented

rpastrana · 2017-08-08T14:26:14Z

tools/esdlcmd/esdlcmd_shell.cpp

           "Commonly used commands:\n"
           "   xml               Generate XML from ESDL definition.\n"
           "   ecl               Generate ECL from ESDL definition.\n"
           "   xsd               Generate XSD from ESDL definition.\n"
           "   wsdl              Generate WSDL from ESDL definition.\n"
           "   publish           Publish ESDL Definition for ESP use.\n"
           "   list-definitions  List all ESDL definitions.\n"
+           "   get-definition    Get ESDL definition.\n"


Is this part of a different change?

@rpastrana get-definition command is already there, it was only missing from the usage.

rpastrana

@mayx looks good. I do have a question about the new command "get-definition"

rpastrana

@mayx looks good.

mayx force-pushed the HPCC-17859 branch from 95f9a67 to bdcbb70 Compare July 31, 2017 18:50

rpastrana reviewed Jul 31, 2017

View reviewed changes

mayx force-pushed the HPCC-17859 branch 2 times, most recently from 5b09435 to c427fe7 Compare August 1, 2017 20:54

rpastrana requested changes Aug 2, 2017

View reviewed changes

mayx force-pushed the HPCC-17859 branch from c427fe7 to c48acde Compare August 2, 2017 21:57

afishbeck reviewed Aug 2, 2017

View reviewed changes

afishbeck reviewed Aug 3, 2017

View reviewed changes

afishbeck requested changes Aug 3, 2017

View reviewed changes

mayx force-pushed the HPCC-17859 branch from c48acde to 35c18b4 Compare August 3, 2017 14:19

mayx force-pushed the HPCC-17859 branch from 35c18b4 to 7a0d3de Compare August 7, 2017 15:43

afishbeck reviewed Aug 7, 2017

View reviewed changes

afishbeck approved these changes Aug 7, 2017

View reviewed changes

rpastrana reviewed Aug 8, 2017

View reviewed changes

rpastrana approved these changes Aug 9, 2017

View reviewed changes

richardkchapman merged commit 419fbb5 into hpcc-systems:candidate-6.4.2 Aug 14, 2017

HPCC-18038 Remove the version option from the publish and java command #10316

HPCC-18038 Remove the version option from the publish and java command #10316

Conversation

mayx commented Jul 31, 2017 • edited

Type of change:

Checklist:

Testing:

hpcc-jirabot commented Jul 31, 2017

mayx commented Jul 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afishbeck Jul 31, 2017 • edited

Choose a reason for hiding this comment

mayx Jul 31, 2017 • edited

Choose a reason for hiding this comment

mayx commented Aug 1, 2017

rpastrana left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mayx commented Aug 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afishbeck left a comment

Choose a reason for hiding this comment

mayx commented Aug 3, 2017

mayx commented Aug 7, 2017

HPCCSmoketest commented Aug 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afishbeck left a comment

Choose a reason for hiding this comment

afishbeck left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rpastrana left a comment

Choose a reason for hiding this comment

rpastrana left a comment

Choose a reason for hiding this comment

mayx commented Jul 31, 2017 •

edited

afishbeck Jul 31, 2017 •

edited

mayx Jul 31, 2017 •

edited