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

[v1.10.0] [Enabler] Standardization of choices in modules #1388

Merged
merged 29 commits into from
Apr 16, 2024

Conversation

rexemin
Copy link
Collaborator

@rexemin rexemin commented Apr 3, 2024

SUMMARY

Fixes #1360

ISSUE TYPE
  • Enabler Pull Request
COMPONENT NAME

ibm_zos_core

ADDITIONAL INFORMATION

Copy link
Collaborator

@richp405 richp405 left a comment

Choose a reason for hiding this comment

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

This is an amazing piece of work! Very detailed. I have found 2 issues which may need changing, or I may misunderstand what I saw. Let me know either way.

plugins/module_utils/data_set.py Show resolved Hide resolved
plugins/modules/zos_copy.py Show resolved Hide resolved
@fernandofloresg fernandofloresg marked this pull request as ready for review April 12, 2024 15:21
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.

I don't have any question that is not currently asked by Rich.

plugins/module_utils/data_set.py Show resolved Hide resolved
plugins/modules/zos_copy.py Show resolved Hide resolved
Copy link
Collaborator

@ketankelkar ketankelkar left a comment

Choose a reason for hiding this comment

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

LGTM- amazing work here Ivan!! Thanks for delivering this mighty effort (which was merely an idea just last week)!!

I've also validated the functionality delivered here while updating (and running) the playbooks in the samples repo.

I think there are some places where stylistic changes could be made, eg:

  • instead of removing a call to upper(), replace it with a call to lower()
  • leaving params in lower-case until upper is required (eg for certain zoau calls)

But, we should make those as part of other development efforts rather than here. The interface has been updated and I saw no gaps in functionality, so I'd consider this issue completed.

@rexemin rexemin requested a review from richp405 April 15, 2024 22:19
Copy link
Collaborator

@richp405 richp405 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 explanations. I wanted to be sure the remaining capitalization was intentional. Nice work!

@fernandofloresg fernandofloresg merged commit 5b239b1 into dev Apr 16, 2024
4 checks passed
@fernandofloresg fernandofloresg deleted the enabler/1360/choices_standardization branch April 16, 2024 15:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants