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

[TSPS-322] make explicit storage container url in PrepareImputationInputsStep #135

Merged
merged 6 commits into from
Sep 25, 2024

Conversation

mmorgantaylor
Copy link
Collaborator

@mmorgantaylor mmorgantaylor commented Sep 23, 2024

Description

The previous conversion from storageContainerUrl to storageContainerName caused a problem in PreparePipelineInputsStep since it expects that value to be the full Url. Here we add a value to the flight working map to specify the protocol to append to the storageContainerName.

Also did a refactor of a bunch of methods related to input and output formatting and generation into a new PipelineInputsOutputsService.

Jira Ticket

https://broadworkbench.atlassian.net/browse/TSPS-332

@mmorgantaylor mmorgantaylor changed the title [bugfix] make explicit storage container url in PrepareImputationInputsStep [TSPS-322] make explicit storage container url in PrepareImputationInputsStep Sep 23, 2024
Copy link
Collaborator

@jsotobroad jsotobroad left a comment

Choose a reason for hiding this comment

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

This looks so much cleaner! One comment about object mapper usage


private final PipelineInputsRepository pipelineInputsRepository;
private final PipelineOutputsRepository pipelineOutputsRepository;
private final ObjectMapper objectMapper = new ObjectMapper();
Copy link
Collaborator

Choose a reason for hiding this comment

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

so i think we want to autowire this in so we can use the one from the bean we create as opposed to the default one (we should do this everywhere we dont already do this)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ah thanks, forgot about that

@@ -8,6 +8,8 @@ public abstract class RunImputationJobFlightMapKeys {
public static final String ALL_PIPELINE_INPUTS = "all_pipeline_inputs";
public static final String CONTROL_WORKSPACE_STORAGE_CONTAINER_NAME =
"control_workspace_storage_container_name";
public static final String CONTROL_WORKSPACE_STORAGE_CONTAINER_PROTOCOL =
"control_workspace_storage_container_protocol";
Copy link
Collaborator

Choose a reason for hiding this comment

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

i wonder what the limit of these strings are in the stairway database hahaha

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

haha yeah we apparently haven't hit the limit yet. want me to find out?

Copy link

sonarcloud bot commented Sep 24, 2024

@mmorgantaylor mmorgantaylor merged commit 971da23 into main Sep 25, 2024
12 checks passed
@mmorgantaylor mmorgantaylor deleted the mma_fix_storage_container_url_use branch September 25, 2024 14: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.

2 participants