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

AJ-916 Clean up shared CBAS-WDS helm logic for standalone WDS #237

Merged
merged 8 commits into from May 9, 2023

Conversation

yuliadub
Copy link
Contributor

@yuliadub yuliadub commented Apr 20, 2023

Clean up to enable standalone WDS, ticket link here.

wds chart now will live here: https://github.com/broadinstitute/terra-helmfile/tree/master/charts/wds

@yuliadub yuliadub marked this pull request as ready for review April 21, 2023 05:32
"cromwellUrlRoot" : "{{ .Values.relay.path }}/cromwell",
"wdsUrlRoot" : "{{ .Values.relay.path }}/wds",
"wdsInstanceId" : "{{ .Values.persistence.workspaceManager.workspaceId }}"
"cromwellUrlRoot" : "{{ .Values.relay.path }}/cromwell"
Copy link
Contributor

Choose a reason for hiding this comment

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

I think cbas will still need these wds values in order to communicate with it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wasnt sure if the .Values.relay.path is still the right url root? after digging around I guess the url hasnt changes, just something else is deploying it, but in this context I guess I am not quite sure if it is safe to assume that Values.relay and Values.ingess are going to have the right path for wds now that the charts are decoupled?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hopefully CBAS is ensuring that the right URL is being passed no matter where WDS is coming from - I think in this ticket but you can check with them if you're not sure.

"cromwellUrlRoot" : "{{ .Values.ingress.path }}/cromwell",
"wdsUrlRoot" : "{{ .Values.ingress.path }}/wds",
"wdsInstanceId" : "{{ .Values.persistence.workspaceManager.workspaceId }}"
"cromwellUrlRoot" : "{{ .Values.ingress.path }}/cromwell"
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto here

@aaronkanzer
Copy link
Contributor

Other than @calypsomatic comments -- this LGTM

Copy link
Contributor

@davidangb davidangb left a comment

Choose a reason for hiding this comment

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

I think this looks right, definitely want to hear from the Workflow(s) team(s) on this one

@calypsomatic calypsomatic self-assigned this May 9, 2023
@calypsomatic calypsomatic merged commit 17645e9 into main May 9, 2023
1 check passed
@calypsomatic calypsomatic deleted the aj-916-remove-wds branch May 9, 2023 17:40
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.

None yet

5 participants