-
Notifications
You must be signed in to change notification settings - Fork 32
I1 #894
base: main
Are you sure you want to change the base?
I1 #894
Conversation
Instead of parsing the CICS.xml. Copy the bundle directory from the Galasa resources directory, copying the bundle parts according to the part type in either binary or text, after applying substitutions.
Automatic triggering of the build is cancelled as user is not a member of the approved groups. Please ask an admin to review this PR and create a comment on the PR stating 'Approved for building' |
Hi @galasa-team I need a review on this change. I can't assign anyone. |
try { | ||
this.runTemporaryUNIXPath = this.zosFileHandler.newUNIXFile(cicsRegion.getRunTemporaryUNIXDirectory().getUnixPath() + "CICSBundles" + SLASH_SYBMOL + getName() + SLASH_SYBMOL, this.cicsZosImage); | ||
this.runTemporaryUNIXPath = this.zosFileHandler.newUNIXFile(cicsRegion.getRunTemporaryUNIXDirectory().getUnixPath() + "CICSBundles" + SLASH_SYBMOL , this.cicsZosImage); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SYMBOL
is spelt badily ;-)
|
||
|
||
|
||
public void copyBundleFilesToZfs() throws ZosUNIXFileException, IOException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pretty complex logic in this method. Any plans to add some unit tests to make sure it does what you expect ?
} | ||
this.cicsRegion.cemt().setResource(this.cicsTerminal, RESOURCE_TYPE_BUNDLE, getName(), "DISABLED"); | ||
this.cicsRegion.cemt().setResource(this.cicsTerminal, RESOURCE_TYPE_BUNDLE, getDefinitionName(), "DISABLED"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what it does, but pls pardon the obvious question: Should "ENABLED" and "DISABLED" be symbolic constants rather than literals, so they can be re-used elsewhere and documented what values are allowed ? If it's only those two possible values, should it be a boolean instead of a string ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not got to try it as there's no jvmserver support for my test bundles. Possibly could be, and maybe more states.
} catch (CicstsManagerException e) { | ||
throw new CicsBundleResourceException("Problem disabling " + RESOURCE_TYPE_BUNDLE + " " + getName(), e); | ||
throw new CicsBundleResourceException("Problem disabling " + RESOURCE_TYPE_BUNDLE + " " + getDefinitionName(), e); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it ever possible that getDefinitionName returns null ? In which case would this cause a NullPtrException ? Better to use one of the formatting APIs like MessageFormat.format(template,parameter...) which copes with nulls as substitution strings ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not got to try it as there's no jvmserver support for my test bundles.
@@ -186,5 +189,8 @@ public interface ICicsBundle { | |||
* Returns the CICS BUNDLE name as defined in the CICS Resource Definition | |||
* @return the CICS BUNDLE name | |||
*/ | |||
public String getName(); | |||
public String getDefinitionName(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Normally, you'd deprecate the public interface method, rather than renaming/removing it.
How sure are we that nobody is going to break as a result of this change ?
Could any of the Galasa users out there be using this interface ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved for building
Approved for building |
What is needed for this to merge ? |
No description provided.