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

Add an option to filter out subworkflows in specific Cromwells #698

Merged
merged 5 commits into from
Sep 9, 2020

Conversation

rexwangcc
Copy link
Contributor

@rexwangcc rexwangcc commented Sep 8, 2020

Reason

In general, we are deploying JMUI against a Cromwell instance that uses OAuth (an OIDC based AuthProxy) but does not have CromIAM. This is affected by #576 that subworkflows are showing up in the job details page.

See discussion here: https://broadinstitute.slack.com/archives/CFA6Q2QBU/p1599146973203800?thread_ts=1599145468.201200&cid=CFA6Q2QBU

Changes

Add a INCLUDE_SUBWORKFLOWS flag and set it to True by default so subworkflows are filtered out by default as well. non-CromIAM Cromwell backends that are using OAuth should set it to False.

@rexwangcc rexwangcc changed the title Fix a regression that mess up subworkflows Fix a regression that mess up subworkflows in specific Cromwells Sep 8, 2020
@rexwangcc rexwangcc changed the title Fix a regression that mess up subworkflows in specific Cromwells Fix an issue that messes up subworkflows in specific Cromwells Sep 8, 2020
Copy link
Contributor

@samanehsan samanehsan 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 great, thanks! :octocat:

Copy link
Contributor

@rsasch rsasch left a comment

Choose a reason for hiding this comment

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

Nit: if the purpose of the new config option is just to not include sub-workflows in some cases, I think naming the option IS_CROMIAM is confusing (unless you anticipate other logic will need to change based on whether or not CromIAM is involved).

@rexwangcc
Copy link
Contributor Author

Nit: if the purpose of the new config option is just to not include sub-workflows in some cases, I think naming the option IS_CROMIAM is confusing (unless you anticipate other logic will need to change based on whether or not CromIAM is involved).

@rsasch
Thanks! That is a good point, at least for now the only purpose is to filter out sub-workflows by default. Do you think renaming it to "INCLUDE_SUBWORKFLOWS" and set it to False is safe? (safe means this won't break any Terra components/logic)

@rsasch
Copy link
Contributor

rsasch commented Sep 9, 2020

Thanks! That is a good point, at least for now the only purpose is to filter out sub-workflows by default. Do you think renaming it to "INCLUDE_SUBWORKFLOWS" and set it to False is safe? (safe means this won't break any Terra components/logic)

That name works for me, but I think it should be true by default since Terra shows sub-workflows on the job details page.

@rexwangcc
Copy link
Contributor Author

rexwangcc commented Sep 9, 2020

Thanks! That is a good point, at least for now the only purpose is to filter out sub-workflows by default. Do you think renaming it to "INCLUDE_SUBWORKFLOWS" and set it to False is safe? (safe means this won't break any Terra components/logic)

That name works for me, but I think it should be true by default since Terra shows sub-workflows on the job details page.

Great, thanks! TIL that by default Terra shows sub-workflows on the job details page, since we prefer the opposite in daily operations 😄

@rexwangcc rexwangcc merged commit 024b38f into master Sep 9, 2020
@rexwangcc rexwangcc deleted the rex/fix-a-regression-that-mess-up-subworkflows branch September 9, 2020 20:11
@rexwangcc rexwangcc changed the title Fix an issue that messes up subworkflows in specific Cromwells Add an option to filter out subworkflows in specific Cromwells Sep 9, 2020
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.

3 participants