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

[bugfix][1678]Add validation for maker begin end #1684

Merged
merged 11 commits into from
Sep 24, 2024

Conversation

AndreMarcel99
Copy link
Collaborator

SUMMARY

Add documentation, test case and validation for user

Fixes #1678

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

Add validation
Captura de pantalla 2024-09-10 a la(s) 12 28 42 p m
Captura de pantalla 2024-09-10 a la(s) 12 28 58 p m

@AndreMarcel99 AndreMarcel99 changed the title [bugfix][1678] [bugfix][1678]Add validation for maker begin end Sep 10, 2024
Copy link
Collaborator

@fernandofloresg fernandofloresg left a comment

Choose a reason for hiding this comment

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

Thank you for the quick addition for the validation, just a few corrections and don't think the test case is necessary as it is, rather, I would like to see a negative test case that checks the validation when both markers are the same, no need to setup a uss env, just call the module and validate the error.

plugins/modules/zos_blockinfile.py Outdated Show resolved Hide resolved
plugins/modules/zos_blockinfile.py Outdated Show resolved Hide resolved
plugins/modules/zos_blockinfile.py Outdated Show resolved Hide resolved
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think there is need for this new test case, looks like is doing the same as test_uss_block_absent_custommarker

Copy link
Collaborator

@fernandofloresg fernandofloresg left a comment

Choose a reason for hiding this comment

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

Sorry, asking for a redundant change.

plugins/modules/zos_blockinfile.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@fernandofloresg fernandofloresg left a comment

Choose a reason for hiding this comment

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

Thanks for the changes.

bugfixes:
- zos_blockinfile - User could set same values for marker begin and end generate issue on ZOAU python API.
Change now validates that ``marker_begin`` and ``marker_end`` are different and fails if they are the same.
(https://github.com/ansible-collections/ibm_zos_core/pull/1684).
Copy link
Collaborator

Choose a reason for hiding this comment

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

An Ansible user won't care particularly about bugs in ZOAU (or any other dependencies). Better to use this space to describe the bug as experienced by the Ansible user.

User could previously set the same values for module options 'marker_begin' and 'marker_end', which would result in <..whatever the issue was..>. Fix now introduces a requirement for 'marker_begin' and 'marker_end' to have different values.

Even better would be if you could bring the <..whatever the issue was..> to the beginning,

Previously <..issue..> happened when 'marker_begin' and 'marker_end' were set to the same value. Fix introduces a requirement for 'marker_begin' and 'marker_end' to have different values.

required: false
type: str
default: BEGIN
marker_end:
required: false
description:
- This will be inserted at C({mark}) in the closing ansible block marker.
- Required unique value different from marker_begin.
Copy link
Collaborator

Choose a reason for hiding this comment

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

seems kind of strange to have a different description here than in marker_begin. I'd say pick one or the other and use the same line in both module options

If you go with "Value needs to be different from marker_end." I'd say use stronger language "Value must be different from marker_end."

Copy link
Collaborator

@fernandofloresg fernandofloresg left a comment

Choose a reason for hiding this comment

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

Agree with @ketankelkar comments and request a change at least in the docs.

Copy link
Collaborator

@fernandofloresg fernandofloresg left a comment

Choose a reason for hiding this comment

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

Requested a correction for a wrong dot, now committed it from GitHub to not have you get back to this.

@fernandofloresg fernandofloresg merged commit 5ad5ac8 into dev Sep 24, 2024
5 checks passed
@fernandofloresg fernandofloresg deleted the bugfix/1678/absent_using_markers_begin_end branch September 24, 2024 16:13
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.

[Bug]Zos_blockinfile_not use marker_begin and end
4 participants