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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 1 commit into from
Aug 14, 2017
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions clienttools/IDEPlugins/ESDL/esdl.bat.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,8 +60,8 @@ for /f "tokens=1,2,3 delims=/:" %%a in ("%8") do set http=%%a&set server=%%b&set
echo.server: %server% >> %TMP%\log.txt
echo.port : %port% >> %TMP%\log.txt

echo "%clienttoolsbindir%\esdl.exe publish %newinput% %2 --server %server% --port %port% --version 1.0 --username %6 --password %9 -v" >>%TMP%\log.txt
"%clienttoolsbindir%"\esdl.exe publish %newinput% %2 --server %server% --port %port% --version 1.0 --username %6 --password %9 -v 2>%5 >>%TMP%\log.txt
echo "%clienttoolsbindir%\esdl.exe publish %newinput% %2 --server %server% --port %port% --username %6 --password %9 -v" >>%TMP%\log.txt
"%clienttoolsbindir%"\esdl.exe publish %newinput% %2 --server %server% --port %port% --username %6 --password %9 -v 2>%5 >>%TMP%\log.txt
goto end

:genecl
Expand Down
2 changes: 1 addition & 1 deletion esp/esdllib/esdl_def.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1662,7 +1662,7 @@ void EsdlDefinition::walkDefinitionDepthFirst( AddedObjs& foundByName, EsdlDefOb
// ancestors- they've already been added.
// Checking up front here will also allow us to detect cycles in the strcuture
// graph -such as we have with BpsReportRelative- and avoid an infinite loop
if( found || esdlObj->checkOptional(opts) == false || esdlObj->checkVersion(requestedVer) == false )
if (found || esdlObj->checkOptional(opts) == false || (requestedVer != 0.0 && esdlObj->checkVersion(requestedVer) == false))
{
//DBGLOG("%s</%s><Skipped/>", indent.str(), esdlObj->queryName());
return;
Expand Down
4 changes: 2 additions & 2 deletions initfiles/examples/EsdlExample/ReadMeFirst.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ Make a copy of the /opt/HPCCSystems/examples/EsdlExample folder.
From EsdlExample copy folder:

1. Generate java base classes (and dummy implementation file):
esdl java esdl_example.esdl EsdlExample --version=9
esdl java esdl_example.esdl EsdlExample

2. Compile java base classes and example service (must have sudo access to place the classes in the default HPCC class location):
sudo javac -g EsdlExampleServiceBase.java -cp /opt/HPCCSystems/classes -d /opt/HPCCSystems/classes/
Expand All @@ -37,7 +37,7 @@ esdl ecl esdl_example.esdl .
ecl publish roxie RoxieEchoPersonInfo.ecl

5. Publish the esdl defined service to dynamicESDL:
esdl publish esdl_example.esdl EsdlExample --version 9 --overwrite
esdl publish esdl_example.esdl EsdlExample --overwrite

5. Bind both java and roxie implementations to DynamicESDL
esdl bind-service myesp 8088 esdlexample.1 EsdlExample --config esdl_binding.xml --overwrite
Expand Down
87 changes: 21 additions & 66 deletions tools/esdlcmd/esdl-publish.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,6 @@ class EsdlPublishCmdCommon : public EsdlCmdCommon
StringAttr optService;
StringAttr optWSProcAddress;
StringAttr optWSProcPort;
StringAttr optVersionStr;
double optVersion;
StringAttr optUser;
StringAttr optPass;
StringAttr optESDLDefID;
Expand Down Expand Up @@ -79,7 +77,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"
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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?

);
EsdlCmdCommon::usage();
}
Expand All @@ -94,8 +91,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))
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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...

Copy link
Member

Choose a reason for hiding this comment

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

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)

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree with moving --version as a command.

return true;
if (iter.matchFlag(optUser, ESDL_OPT_SERVICE_USER) || iter.matchFlag(optUser, ESDL_OPTION_SERVICE_USER))
return true;
if (iter.matchFlag(optPass, ESDL_OPT_SERVICE_PASS) || iter.matchFlag(optPass, ESDL_OPTION_SERVICE_PASS))
Expand Down Expand Up @@ -127,7 +122,7 @@ class EsdlPublishCmd : public EsdlPublishCmdCommon
Owned<IClientPublishESDLDefinitionRequest> request = esdlConfigClient->createPublishESDLDefinitionRequest();

StringBuffer esxml;
esdlHelper->getServiceESXDL(optSource.get(), optESDLService.get(), esxml, optVersion);
esdlHelper->getServiceESXDL(optSource.get(), optESDLService.get(), esxml, 0);

if (esxml.length()==0)
{
Expand All @@ -152,7 +147,7 @@ class EsdlPublishCmd : public EsdlPublishCmdCommon
return 1;
}

fprintf(stdout, "\nESDL Service: %s(%f): %s", optESDLService.get() ,optVersion, resp->getStatus().getDescription());
fprintf(stdout, "\nESDL Service: %s: %s", optESDLService.get(), resp->getStatus().getDescription());

return 0;
}
Expand Down Expand Up @@ -229,19 +224,6 @@ class EsdlPublishCmd : public EsdlPublishCmdCommon

bool finalizeOptions(IProperties *globals)
{
if (!optVersionStr.isEmpty())
{
optVersion = atof( optVersionStr.get() );
if( optVersion <= 0 )
{
throw MakeStringException( 0, "Version option must be followed by a real number > 0" );
}
}
else
{
fprintf(stderr, "\nWARNING: ESDL Version not specified.\n");
}

if (optSource.isEmpty())
throw MakeStringException( 0, "Source ESDL definition file (ecm|esdl|xml) must be provided" );

Expand Down Expand Up @@ -558,12 +540,9 @@ class EsdlDeleteESDLDefCmd : public EsdlPublishCmdCommon
Owned<IClientWsESDLConfig> esdlConfigClient = EsdlCmdHelper::getWsESDLConfigSoapService(optWSProcAddress, optWSProcPort, optUser, optPass);
Owned<IClientDeleteESDLDefinitionRequest> request = esdlConfigClient->createDeleteESDLDefinitionRequest();

fprintf(stdout,"\nAttempting to delete ESDL definition: '%s.%d'\n", optESDLService.get(), (int)optVersion);
fprintf(stdout,"\nAttempting to delete ESDL definition: '%s'\n", optESDLDefID.get());

StringBuffer id;
id.setf("%s.%d", optESDLService.get(), (int)optVersion);

request->setId(id);
request->setId(optESDLDefID.get());

Owned<IClientDeleteESDLRegistryEntryResponse> resp = esdlConfigClient->DeleteESDLDefinition(request);

Expand All @@ -581,18 +560,17 @@ class EsdlDeleteESDLDefCmd : public EsdlPublishCmdCommon
void usage()
{
printf( "\nUsage:\n\n"
"esdl delete <ESDLServiceDefinitionName> <ESDLServiceDefinitionVersion> [command options]\n\n"
" 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");
Copy link
Member

Choose a reason for hiding this comment

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

@JamesDeFabia note this change. It needs to be documented


EsdlPublishCmdCommon::usage();

printf( "\n Use this command to delete an ESDL Service definition.\n"
" To delete an ESDL Service definition, provide the definition name and version\n"
" To delete an ESDL Service definition, provide the ESDL definition id\n"
);

printf("\nExample:"
">esdl delete myesdldef 5\n"
">esdl delete myesdldef.5\n"
);
}

Expand All @@ -604,18 +582,15 @@ class EsdlDeleteESDLDefCmd : public EsdlPublishCmdCommon
return false;
}

for (int cur = 0; cur < 2 && !iter.done(); cur++)
for (int cur = 0; cur < 1 && !iter.done(); cur++)
{
const char *arg = iter.query();
if (*arg != '-')
{
switch (cur)
{
case 0:
optESDLService.set(arg);
break;
case 1:
optVersionStr.set(arg);
optESDLDefID.set(arg);
break;
}
}
Expand Down Expand Up @@ -652,19 +627,8 @@ class EsdlDeleteESDLDefCmd : public EsdlPublishCmdCommon
bool finalizeOptions(IProperties *globals)
{

if (optESDLService.isEmpty())
throw MakeStringException( 0, "Name of ESDL service definition must be provided!" );

if (!optVersionStr.isEmpty())
{
optVersion = atof( optVersionStr.get() );
if( optVersion <= 0 )
{
throw MakeStringException( 0, "Version option must be followed by a real number > 0" );
}
}
else
throw MakeStringException( 0, "ESDL service definition version must be provided!" );
if (optESDLDefID.isEmpty())
throw MakeStringException( 0, "ESDLDefinitionID must be provided!" );

return EsdlPublishCmdCommon::finalizeOptions(globals);
}
Expand Down Expand Up @@ -823,8 +787,7 @@ class EsdlBindMethodCmd : public EsdlBindServiceCmd
request->setEspProcName(optTargetESPProcName);
request->setEspBindingName(optBindingName);
request->setEsdlServiceName(optService.get());
VStringBuffer id("%s.%d", optService.get(), (int)optVersion);
request->setEsdlDefinitionID(id.str());
request->setEsdlDefinitionID(optESDLDefID.get());
request->setConfig(optInput);
request->setOverwrite(optOverWrite);

Expand All @@ -847,11 +810,11 @@ class EsdlBindMethodCmd : public EsdlBindServiceCmd
void usage()
{
printf( "\nUsage:\n\n"
"esdl bind-method <TargetESPProcessName> <TargetESPBindingName> <TargetServiceName> <TargetServiceDefVersion> <TargetMethodName> [command options]\n\n"
"esdl bind-method <TargetESPProcessName> <TargetESPBindingName> <TargetServiceName> <TargetESDLDefinitionID> <TargetMethodName> [command options]\n\n"
" TargetESPProcessName The target ESP Process name\n"
" TargetESPBindingName The target ESP binding name associated with this service\n"
" TargetServiceName The name of the Service to bind (must already be defined in dali.)\n"
" TargetServiceDefVersion The version of the target service ESDL definition (must exist in dali)\n"
" TargetESDLDefinitionID The id of the target ESDL definition (must exist in dali)\n"
" TargetMethodName The name of the target method (must exist in the service ESDL definition)\n"

"\nOptions (use option flag followed by appropriate value):\n"
Expand All @@ -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"
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

@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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

" and the name of the service you are binding.\n"
" Optionally provide configuration information either directly, or via a\n"
" configuration file in the following syntax:\n"
Expand All @@ -874,7 +837,7 @@ class EsdlBindMethodCmd : public EsdlBindServiceCmd
);

printf("\nExample:"
">esdl bind-service myesp 8088 WsMyService --config /myService/methods.xml\n"
">esdl bind-method myesp mybinding WsMyService WsMyService.1 myMethod --config /myService/methods.xml\n"
);
}

Expand Down Expand Up @@ -904,7 +867,7 @@ class EsdlBindMethodCmd : public EsdlBindServiceCmd
optService.set(arg);
break;
case 3:
optVersionStr.set(arg);
optESDLDefID.set(arg);
break;
case 4:
optMethod.set(arg);
Expand Down Expand Up @@ -947,23 +910,15 @@ class EsdlBindMethodCmd : public EsdlBindServiceCmd
}
}

if (!optVersionStr.isEmpty())
{
optVersion = atof( optVersionStr.get() );
if( optVersion <= 0 )
{
throw MakeStringException( 0, "Version option must be followed by a real number > 0" );
}
}
else
throw MakeStringException( 0, "ESDL service definition version must be provided!" );

if(optTargetESPProcName.isEmpty())
throw MakeStringException( 0, "Name of Target ESP process must be provided" );

if (optService.isEmpty())
throw MakeStringException( 0, "Name of ESDL based service must be provided" );

if (optESDLDefID.isEmpty())
throw MakeStringException( 0, "ESDLDefinitionID must be provided!" );

if (optMethod.isEmpty())
throw MakeStringException( 0, "Name of ESDL based method must be provided" );

Expand Down
2 changes: 2 additions & 0 deletions tools/esdlcmd/esdlcmd_common.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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_INTERFACE_VERSION_S "-iv"
#define ESDLOPT_SERVICE "--service"
#define ESDLOPT_METHOD "--method"
#define ESDLOPT_PREPROCESS_OUT "--preprocess-output"
Expand Down