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

fixup: allow Applid to include special chars #144

Merged

Conversation

andrewhughes101
Copy link
Collaborator

SUMMARY

Allows a user to specify an Applid that includes special chars $@#

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

region_jcl

Copy link
Collaborator

@kiera-bennett kiera-bennett left a comment

Choose a reason for hiding this comment

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

Otherwise looks good!

@@ -300,7 +300,7 @@ def get_arg_defs(self): # type: () -> dict
defs[STEPLIB]["options"][DATA_SETS].update({"elements": "data_set_base"})
defs[DFHRPL]["options"][TOP_DATA_SETS].update({"elements": "data_set_base"})
defs[DFHRPL]["options"][DATA_SETS].update({"elements": "data_set_base"})
self.update_arg_def(defs[APPLID], "qualifier")
self.update_arg_def(defs[APPLID], "dd")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this type definitely right? Looking at this doc, where an applid is between 2-8 characters if this is what we are taking as the standard.
And I think type dd allows for 1 char length?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah DD would allow for 1 char as per its regex r"^[A-Z$#@][A-Z0-9@#$]{0,7}$"
But so would qualifier the one were currently using r"^[A-Z]{1}[A-Z0-9]{0,7}$"

Maybe we should add an additional length check into here somewhere

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah good point. Happy for this to merge as we are only improving the existing then, but should raise issue for the length atleast or just add this onto this PR if you have time? Whatever is easiest

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Lets make an issue for length, suspect its gonna be a trickier one to do

@andrewhughes101 andrewhughes101 merged commit 182837a into ansible-collections:main Oct 28, 2024
4 checks passed
@andrewhughes101 andrewhughes101 deleted the applid-bug branch October 28, 2024 15:45
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.

2 participants