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

[Enabler][Zos Job Submit]Update undocumented argument and import exception #1091

Merged
merged 17 commits into from
Dec 20, 2023

Conversation

AndreMarcel99
Copy link
Collaborator

@AndreMarcel99 AndreMarcel99 commented Dec 12, 2023

SUMMARY

Update temp_path and how to import exceptions.

Fiexes #997 #998 #999

ISSUE TYPE
  • Sanity Issue Pull Request
ADDITIONAL INFORMATION

Remove tmp path from options and only work with source as name of tmp file, because it was undocumented argument for ansible and could be any name, also only in case is local make the tmp file, so didn't expect a dataset or file with the same name on the system. Also make a implementation of imports of exceptions by using new implementation to avoid losing context when an import fails.

Pipeline Without Zos_copy
Captura de pantalla 2023-12-12 a la(s) 4 08 41 p m
Captura de pantalla 2023-12-12 a la(s) 4 09 21 p m

Pipeline with zos_copy

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 changes to avoid tech debt in the future and also add a changelog please.

@@ -902,7 +899,7 @@ def run_module():
from_encoding = parsed_args.get("from_encoding")
to_encoding = parsed_args.get("to_encoding")
# temporary file names for copied files when user sets location to LOCAL
temp_file = parsed_args.get("temp_file")
temp_file = parsed_args.get("src")
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would suggest doing this change instead :

Suggested change
temp_file = parsed_args.get("src")
if location == "LOCAL":
temp_file = parsed_args.get("src")

That way temp_file is indeed a temporary file on the remote and we don't need to worry on the location conditions in the future, if temp_file is None we know we don't have to remove anything.

@@ -1039,7 +1036,7 @@ def run_module():
module.exit_json(**result)

finally:
if temp_file:
if location != "DATA_SET" and location != "USS":
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if location != "DATA_SET" and location != "USS":
if temp_file:

I suggest keeping it this way bc that will help us avoid having to check for state and just remove it if it exists.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It generates a error if the local file is named as a dataset (USER.LOCAL) then the file /user.local will fail that verification

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm sorry I don't get how that relates to this code suggestion, could you explain?

@AndreMarcel99 AndreMarcel99 changed the title Enable/997 999/update sanity test ignore 9 11 [Enabler][Zos Job Submit]Update undocumented argument and import exception Dec 12, 2023
@AndreMarcel99
Copy link
Collaborator Author

Pipeline for all except zos_copy
Captura de pantalla 2023-12-14 a la(s) 10 22 01 a m

Captura de pantalla 2023-12-14 a la(s) 10 21 47 a m

@AndreMarcel99
Copy link
Collaborator Author

Pipeline zos_copy
Captura de pantalla 2023-12-14 a la(s) 2 34 44 p m
Captura de pantalla 2023-12-14 a la(s) 2 35 19 p m

@fernandofloresg
Copy link
Collaborator

I made some extra changes and I'm waiting for the pipeline to finish.

@fernandofloresg
Copy link
Collaborator

Screenshot 2023-12-15 at 12 15 28 PM

@fernandofloresg
Copy link
Collaborator

fernandofloresg commented Dec 15, 2023

In case anyone wonders why iconv section is because when zos_copy module was integrated into zos_job_submit encoding option was not used, this way we can leverage zos_copy encoding capabilities and standardize copying files into remote.

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.

Looks good to me now

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.

Looks good!

@AndreMarcel99 AndreMarcel99 merged commit b4e4139 into dev Dec 20, 2023
@AndreMarcel99 AndreMarcel99 deleted the enable/997-999/update-sanity-test-ignore-9-11 branch December 20, 2023 20:00
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.

4 participants