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

overwrite manually set ids #1210

Draft
wants to merge 9 commits into
base: main
Choose a base branch
from
Draft

overwrite manually set ids #1210

wants to merge 9 commits into from

Conversation

mr-c
Copy link
Member

@mr-c mr-c commented Oct 15, 2019

Solves #1204 but there may be a better way

@mr-c mr-c requested a review from tetron October 15, 2019 09:18
@codecov
Copy link

codecov bot commented Oct 15, 2019

Codecov Report

Merging #1210 into master will decrease coverage by 0.1%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1210      +/-   ##
==========================================
- Coverage   77.18%   77.08%   -0.11%     
==========================================
  Files          35       35              
  Lines        7216     7218       +2     
  Branches     1853     1854       +1     
==========================================
- Hits         5570     5564       -6     
- Misses       1173     1181       +8     
  Partials      473      473
Impacted Files Coverage Δ
cwltool/load_tool.py 84.93% <100%> (+0.13%) ⬆️
cwltool/sandboxjs.py 73.07% <0%> (-5.13%) ⬇️
cwltool/job.py 67.01% <0%> (+0.84%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f997d13...c8ff2b5. Read the comment docs.

@stain
Copy link
Member

stain commented Oct 15, 2019

I am not sure I understand this patch.. so if name is set on the top-level workflow loaded from a *.cwl file, then that becomes its new id? (presumably instead of #main?)

@mr-c
Copy link
Member Author

mr-c commented Oct 15, 2019

@stain name is a reserved field in avro, it gets set during parsing by schema-salad. In the circumstances of this bug it happens to contain the normal computed ID that is being overwritten by the user provided id and breaking the graph.

Like I said, this isn't a great fix, but it works

@mr-c
Copy link
Member Author

mr-c commented Oct 15, 2019

top-level workflow

No, workflowobj is the contents of a workflow step run field after loading the URI reference.

@mr-c
Copy link
Member Author

mr-c commented Oct 15, 2019

Jenkins, test this please

@mr-c mr-c changed the base branch from master to main July 2, 2020 11:21
@mr-c mr-c marked this pull request as draft December 3, 2020 17:51
@lgtm-com
Copy link

lgtm-com bot commented Sep 3, 2021

This pull request introduces 1 alert and fixes 17 when merging a29c642 into b67a5e0 - view on LGTM.com

new alerts:

  • 1 for Syntax error

fixed alerts:

  • 16 for Module-level cyclic import
  • 1 for Clear-text logging of sensitive information

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