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

change type of results_root and inputs_root from args to string #63

Closed

Conversation

zacharyburnett
Copy link
Collaborator

@zacharyburnett zacharyburnett commented Dec 19, 2023

resolves SCSB-114

this change could break downstream if people are assuming that results_root and inputs_root are passed as a list of strings

@zacharyburnett zacharyburnett self-assigned this Dec 19, 2023
@zacharyburnett zacharyburnett changed the title change type of options from args to string change type of results_root and inputs_root from args to string Dec 19, 2023
@zacharyburnett
Copy link
Collaborator Author

Copy link
Collaborator

@pllim pllim left a comment

Choose a reason for hiding this comment

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

Why is this needed, especially if it is going to break existing usages?

@zacharyburnett
Copy link
Collaborator Author

Why is this needed, especially if it is going to break existing usages?

I think we can assume the inputs_root and results_root will always be a single path, and all existing usages of it are indexing the first entry unconditionally anyway.

@pllim
Copy link
Collaborator

pllim commented Dec 21, 2023

Sure, but then all the downstream packages will need to do a version check of ci-watson and handle stuff accordingly. I just don't see the point. What was the motivation for this?

@zacharyburnett
Copy link
Collaborator Author

the JWST regression test jobs were failing at the results parsing stage, because they couldn't correctly parse inputs_root and results_root; however, it looks like this was solved by a PR to jenkins_shared_ci_utils:

You're right, it might be more headache to fix this than it's worth. I'll close this

@zacharyburnett zacharyburnett deleted the root_dirs_string branch December 21, 2023 15:08
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