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

I1 #894

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

I1 #894

wants to merge 3 commits into from

Conversation

ChrisAtkinson1
Copy link

No description provided.

Signed-off-by: atkinchr <atkinchr@uk.ibm.com>
Signed-off-by: atkinchr <atkinchr@uk.ibm.com>
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.
@galasa-team
Copy link
Contributor

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'

@ChrisAtkinson1
Copy link
Author

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);
Copy link
Contributor

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 {
Copy link
Contributor

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");
Copy link
Contributor

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 ?

Copy link
Author

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);
Copy link
Contributor

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 ?

Copy link
Author

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();
Copy link
Contributor

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 ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Approved for building

@techcobweb
Copy link
Contributor

Approved for building

@ChrisAtkinson1
Copy link
Author

What is needed for this to merge ?

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.

None yet

3 participants