#94 -IBM Spectrum Protect Plus Audit Log Workflow #95
Conversation
ChrisCollinsIBM
added
the
workflow-submission
There was a problem hiding this comment.
Great submission overall!
You have a very extensive README.md which is great!
I've added some comments around variable defaults and their usage to look at, if you have any questions or want to discuss please add comments where appropriate.
Thanks for the submission!
|
|
||
| # Workflow parameters | ||
|
|
||
| * #host# (required): IP_Address or Hostname, no protocol prefix, e.g. "mySppHost.myCompany.com" will be assebled to https://mySppHost.myCompany.com:443 |
There was a problem hiding this comment.
| * #host# (required): IP_Address or Hostname, no protocol prefix, e.g. "mySppHost.myCompany.com" will be assebled to https://mySppHost.myCompany.com:443 | |
| * #host# (required): IP_Address or Hostname, no protocol prefix, e.g. "mySppHost.myCompany.com" will be assembled to https://mySppHost.myCompany.com:443 |
| # Workflow parameters | ||
|
|
||
| * #host# (required): IP_Address or Hostname, no protocol prefix, e.g. "mySppHost.myCompany.com" will be assebled to https://mySppHost.myCompany.com:443 | ||
| * #port# (not required, default is 443): 443 |
There was a problem hiding this comment.
Here the port claims to be not required however the parameter is required on line 7 in IBM Spectrum Protect Plus Auditlog/spp-Workflow.xml
<Parameter name="port" label="port" required="true" />
I've added some suggestions on handling this near the sppaddress construction.
|
|
||
| <Actions> | ||
| <Set path="/debug" value="false" /> | ||
| <Set path="/sppaddress" value="https://${/host}:${/port}" /> |
There was a problem hiding this comment.
Have you considered adding logic when constructing sppaddress to only append the port if it's not empty or isn't 443? https:// doesn't need a port added to the URL if it's 443
|
|
||
| <Parameters> | ||
| <Parameter name="host" label="host" required="true" /> | ||
| <Parameter name="port" label="port" required="true" /> |
There was a problem hiding this comment.
As mentioned above the readme says this parameter is optional but marked required here. Additional comments on line 15
| --> | ||
| <If condition="/firstApiCall = true"> | ||
| <CallEndpoint url="${/next_page}" method="GET" savePath="/response"> | ||
| <QueryParameter name="pageSize" value="${/pageSize}" omitIfEmpty="true" /> |
There was a problem hiding this comment.
pageSize is defined on line 10 as required="false" - what will happen here if that value isn't set?
| <Value name="port" value="443"/> | ||
| <Value name="username" value="monitorUser" /> | ||
| <Value name="userpwd" value="Passw0rd" /> | ||
| <Value name="pageSize" value="100" /> |
There was a problem hiding this comment.
pageSize is camel case and the rest are all lowercase. Not a big deal but might want to make them consistent.
|
|
||
| <!-- Get List of logs with API token --> | ||
| <DoWhile condition="/next_page != null"> | ||
| <Log type="INFO" message="SPP IQUCRA> URL: ${/next_page}" /> |
There was a problem hiding this comment.
This one might be more of a candidate for DEBUG rather than INFO
| </Else> | ||
|
|
||
| <Log type="INFO" message="SPP IQUCRA> bookmark (start): ${/bookmark}" /> | ||
| <Log type="INFO" message="SPP IQUCRA> counter (start): ${/counter}" /> |
There was a problem hiding this comment.
This one might be more of a candidate for DEBUG rather than INFO
| <!-- get timestamp of last item in the itemList (in this loop) --> | ||
| <Set path="/lastItem" value="${count(/itemList) -(1)}" /> | ||
| <Set path="/bookmark" value="${/itemList[/lastItem]/accessTime}" /> | ||
| <Log type="INFO" message="SPP IQUCRA> bookmark (queryPage): ${/bookmark}" /> |
There was a problem hiding this comment.
This one might be more of a candidate for DEBUG rather than INFO
| </Actions> | ||
|
|
||
| <Tests> | ||
| <TCPConnectionTest host="${/host}" /> |
There was a problem hiding this comment.
It would be a good idea to include the DNS and the SSL Handshake tests here as well.
|
Changes have been requested prior to merging this workflow, please let me know if you have any questions or require any assistance. Thanks! |
|
Just wanted to follow up to see if you have any questions or comments on the requested changes. It has been inactive for quite some time so I wanted to follow up. Thanks! |
1 similar comment
|
Just wanted to follow up to see if you have any questions or comments on the requested changes. It has been inactive for quite some time so I wanted to follow up. Thanks! |
|
Code review requests are still outstanding on this item, if they addressed then we can get them merged but we'll be closing this for inactivity soon and it can be re-opened if needed. |
issue #94 was opened for this pull request to publish an "IBM Spectrum Protect Plus Audit Log Workflow"