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] [module_utils] [multiple modules] tmp_hlq use in utils whenever mvscmd gets called #1695

Merged
merged 32 commits into from
Oct 4, 2024

Conversation

rexemin
Copy link
Collaborator

@rexemin rexemin commented Sep 18, 2024

SUMMARY

Fixes #912.

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME
  • module_utils/data_set.py
ADDITIONAL INFORMATION

Includes a lot of small changes just to pass the tmp_hlq used by modules all the way down to either DataSet or DataSetUtils where it can be used every time we make a call to mvscmd.

@rexemin rexemin changed the title Bugfix/912/tmp hlq in data set exists [Bugfix] [module_utils] [multiple modules] tmp_hlq use in utils whenever mvscmd gets called Sep 18, 2024
@rexemin rexemin marked this pull request as ready for review September 20, 2024 01:03
@rexemin rexemin added the Do not Merge When a pull request should not be merged for issue noted reasons label Sep 20, 2024
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.

Nice and thorough.

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.

really good work here, i liked your core fix to leveraging the -Q CLI option for specifying temporary high level qualifiers while we wait for the python api to become availble.
image

Change requests are:

  • a note on the focus of the changelog
  • a question about potentially overlooked function calls in the data set module

elif state == "uncataloged":
changed = data_set.ensure_uncataloged()
changed = data_set.ensure_uncataloged(tmp_hlq=tmp_hlq)
return changed
Copy link
Collaborator

Choose a reason for hiding this comment

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

how come the rest of these ensure_* methods don't get the tmphlq passed in? I noticed for example for ensure_absent, you added a default value of tmphlq=None in the function signature, but what if the user wants to define one, how does that value make it to the function call?

Copy link
Collaborator Author

@rexemin rexemin Sep 24, 2024

Choose a reason for hiding this comment

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

Are you only referring to the calls in zos_data_set or also to the functions in the data_set.py util?

The calls in zos_data_set only have tmp_hlq when they deal with common datasets, the calls that check members or GDGs don't end up using mvscmd and thus don't include the HLQ. For the specific call to data_set.ensure_absent, I included the HLQ at first, but then that caused errors while testing, so I removed it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I was referring to:

  • ensure_present
  • ensure_absent
  • ensure_cataloged <-- you've got this one covered
  • ensure_uncataloged <-- you've got this one covered

For ensure_absent, it calls attempt_catalog_if_necessary_and_delete which calls data_set_cataloged which calls mvscmd.

For ensure_present, it calls attempt_catalog_if_necessary which calls data_set_cataloged which calls mvscmd.

To me, it seems like both ensure_present and ensure_absent should pass thetmphlq all the way down the chain, otherwise, the module option would still be ignored for certain operations.
Any chance that the errors you got were because the chain of calls wasn't completely updated?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure what other updates I made to the code, but now the calls don't end up with errors.

changelogs/fragments/1695-tmp_hlq_when_calling_mvscmd.yml Outdated 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.

to me, it seems like ensure_present and ensure_absent in the data set module needs to accomodate tmphlq

See my reply to the comment above (link)

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 - thanks for making the requested changes!

@rexemin
Copy link
Collaborator Author

rexemin commented Oct 2, 2024

Jenkins results:


Screenshot 2024-10-02 at 11 20 51
Screenshot 2024-10-02 at 11 21 04
Screenshot 2024-10-02 at 11 21 20


Screenshot 2024-10-02 at 11 21 31
Screenshot 2024-10-02 at 11 21 43


Screenshot 2024-10-02 at 12 39 14
Screenshot 2024-10-02 at 12 39 23
Screenshot 2024-10-02 at 12 39 37

@rexemin
Copy link
Collaborator Author

rexemin commented Oct 2, 2024

And the rest of the modules affected by the change:


Screenshot 2024-10-02 at 15 26 55
Screenshot 2024-10-02 at 15 27 05
Screenshot 2024-10-02 at 15 27 16


Screenshot 2024-10-02 at 15 28 11
Screenshot 2024-10-02 at 15 28 21
Screenshot 2024-10-02 at 15 28 32


Screenshot 2024-10-02 at 17 09 12
Screenshot 2024-10-02 at 17 09 20
Screenshot 2024-10-02 at 17 09 34


Screenshot 2024-10-02 at 17 36 28
Screenshot 2024-10-02 at 17 36 37
Screenshot 2024-10-02 at 17 36 51


Screenshot 2024-10-03 at 10 51 40
Screenshot 2024-10-03 at 10 51 49
Screenshot 2024-10-03 at 10 52 08

@rexemin rexemin removed the Do not Merge When a pull request should not be merged for issue noted reasons label Oct 4, 2024
@fernandofloresg fernandofloresg merged commit 5c05a25 into dev Oct 4, 2024
5 checks passed
@fernandofloresg fernandofloresg deleted the bugfix/912/tmp_hlq_in_data_set_exists branch October 4, 2024 21:16
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] [module_utils] data_set_exists() does not allow for tmp_hlq
5 participants