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

WX-833 Turn on DRS in Cromwell on Azure, add Martha URL to config #127

Merged
merged 7 commits into from
Nov 22, 2022

Conversation

jgainerdewar
Copy link
Contributor

No description provided.

@@ -11,6 +11,7 @@
disableBatchScheduling: false
dockerInDockerImageName: ""
usePreemptibleVmsOnly: false
marthaUrl: RUNTIME_PARAMETER
Copy link
Contributor

@rtitle rtitle Nov 21, 2022

Choose a reason for hiding this comment

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

Nit -- can we name this drsUrl intead of marthaUrl? I think ID-team is trying to cut Leo-apps over to DRSHub. Right now Leo has a config which tells it whether to send the Martha or DRSHub url when deploying apps (see: DataBiosphere/leonardo#2997).

Copy link
Contributor

@rtitle rtitle Nov 21, 2022

Choose a reason for hiding this comment

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

cc @andy7i @tlangs for your cutover planning, Cromwell-as-an-app will soon also be accepting a DRS URL from Leo :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure thing!

Copy link

Choose a reason for hiding this comment

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

Oh! That might make things much easier. What's the timeline for this?

Copy link
Contributor

Choose a reason for hiding this comment

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

So this change is just for Cromwell-as-an-app on Azure -- not sure the timeline for cutting over Cromwell-as-an-app on GCP, or Cromwell-not-as-an-app-on-GCP (but those should probably be tracked/planned separately).

Copy link

Choose a reason for hiding this comment

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

Ah, darn. Guess I do have to do work then! 😅

Copy link
Contributor

@rtitle rtitle left a comment

Choose a reason for hiding this comment

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

:shipit:

@jgainerdewar
Copy link
Contributor Author

Waiting to merge until after broadinstitute/cromwell#6952, which is required to enable DRS access.

@rtitle
Copy link
Contributor

rtitle commented Nov 22, 2022

Leo side of this: DataBiosphere/leonardo#3036

Needs testing.

- name: MarthaUrl
value: {{ .Values.config.drsUrl }}
- name: CromwellDrsLocalizerImageName
value: {{ .Values.cromwell.image | replace "cromwell" "cromwell-drs-localizer" }}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that DRS resolution in TES isn't tested yet because TES is not working in the app.

@jgainerdewar jgainerdewar merged commit 277caf9 into main Nov 22, 2022
@jgainerdewar jgainerdewar deleted the jgd_WX-833_azuredrs branch November 22, 2022 22:02
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