-
Notifications
You must be signed in to change notification settings - Fork 44
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
[Bug][zos_apf] Ensure idempotency in zos_apf when a library is added or removed #1893
Conversation
* Added encode fix * Added changelogs * Fixed users utility
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.
Just a couple of comments.
@@ -0,0 +1,3 @@ | |||
bugfixes: | |||
- zos_apf - if dataset present in the apf list it will not fail the command on addition or deletion to the list |
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.
We try to (mostly) stick to the ansible development lifecycle guidelines. https://docs.ansible.com/ansible/latest/community/development_process.html, for bugfixes recommendation is to Use past tense to describe the problem and present tense to describe the fix.
.
When trying to add a library into the APF list that was already added the module would fail. Fix now won't fail the module and inform the user that the library was already in the APF list.
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.
Added the change logs as per the instructions
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.
I would suggest getting rid of the contraction: ...'Fix now will not fail the module, and will inform the user that the library is alread on the APF list."
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.
Thank you @richp405 That sounds better. @Rohitcodes28 Can you update it ?
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.
Sure @fernandofloresg updated as per the suggestion.
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.
Nice start! There are suggestions for the changelog fragment and a code recommendation for zos_apf.py. Glad to see you starting to do code already.
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.
Thanks for updating this just have another request, in this case when is added and is already present changed
should be false
.
{
"apf_result": {
"changed": true,
"failed": false,
"rc": 0,
"stderr": "BGYSC4710E Add error: Dataset OMVSADM.TEST.TOUCH2 on volume 333333 is already present in APF list.\n",
"stderr_lines": [
"BGYSC4710E Add error: Dataset OMVSADM.TEST.TOUCH2 on volume 333333 is already present in APF list."
],
"stdout": "",
"stdout_lines": []
}
}
For the negative case, it should be the same case, when you try to remove it from the APF list and is not present, show changed
false.
Sorry did not notice this before.
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.
Just requested one change to the test case, overall is looking good. Please run the test cases in the ansible core pipeline. As an advice, zos_apf can fail in some volumes, so use the same EC you are using to test locally.
# Return code 16 if ZOAU < 1.2.0 and RC is 8 if ZOAU >= 1.2.0 | ||
assert result.get("rc") == 16 or result.get("rc") == 8 | ||
assert result.get("rc") == 0 or result.get("rc") == 16 or result.get("rc") == 8 |
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.
Could you do these checks by zoau version? Like if version 1.3.4 assert rc == 0 if lower rc == 16 or 8 .
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.
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.
LGTM!
Thanks for addressing the requested changes. |
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.
One spelling issue in the changelog file. Code looks fine.
SUMMARY
Added ignore error in the apf add and delete
Fixes #1662
ISSUE TYPE
COMPONENT NAME
zos_apf
ADDITIONAL INFORMATION