# Require fields listed in icSourceFieldsToCopy to be present

XMLWordPrintable

## Details

• Type: Improvement
• Status: Done
• Resolution: Done
• Fix Version/s: None
• Component/s:
• Labels:
• Story Points:
1
• Sprint:
• Team:

## Description

CalibrateTask presently treats config field icSourceFieldsToCopy as a list of fields to copy if present. This was required because one of the standard fields to copy was usually missing. However, Paul Price fixed that problem in DM-5385. Now we can raise an exception if any field listed is missing (though I propose to continue ignoring icSourceFieldsToCopy if isSourceCatalog is not provided).

## Activity

Russell Owen created issue -
Field Original Value New Value
Link This issue is triggered by DM-5385 [ DM-5385 ]
 Status To Do [ 10001 ] In Progress [ 3 ]
Hide
Russell Owen added a comment -

Do you have time to look at this? It's a pretty small change. It is a nice improvement; thank you for making it possible.

I realize that I indented the docs for icSourceFieldsToCopy and modified them in the same commit. I'll separate that out if you like, but I was hoping you'd be willing to let it go, since it's a small change and I didn't think of it until it was too late to make disentangling easy.

Show
Russell Owen added a comment - Do you have time to look at this? It's a pretty small change. It is a nice improvement; thank you for making it possible. I realize that I indented the docs for icSourceFieldsToCopy and modified them in the same commit. I'll separate that out if you like, but I was hoping you'd be willing to let it go, since it's a small change and I didn't think of it until it was too late to make disentangling easy.
 Reviewers Paul Price [ price ] Status In Progress [ 3 ] In Review [ 10004 ]
Hide
Paul Price added a comment -

I think the warning "copyIcSourceFields doing nothing because icSourceFieldsToCopy is empty" is a bit over the top — I even get the warning if I deliberately leave that config parameter empty.

Why choose to raise TaskError? That will suppress the traceback and make the solution harder to find for the user.

Show
Paul Price added a comment - I think the warning "copyIcSourceFields doing nothing because icSourceFieldsToCopy is empty" is a bit over the top — I even get the warning if I deliberately leave that config parameter empty. Why choose to raise TaskError ? That will suppress the traceback and make the solution harder to find for the user.
 Status In Review [ 10004 ] Reviewed [ 10101 ]
Hide
Russell Owen added a comment - - edited

Regarding the warning: you won't see it if icSourceFieldsToCopy is empty if you call calibrate or run because calibrate doesn't call copyIcSourceFields if that config parameter has no elements. You will only see the warning if you call copyIcSourceFields directly, in which case I think it is appropriate – you have asked it to copy fields, but there are no fields to copy.

Regarding the exception: I agree. How about RuntimeError?

Show
Russell Owen added a comment - - edited Regarding the warning: you won't see it if icSourceFieldsToCopy is empty if you call calibrate or run because calibrate doesn't call copyIcSourceFields if that config parameter has no elements. You will only see the warning if you call copyIcSourceFields directly, in which case I think it is appropriate – you have asked it to copy fields, but there are no fields to copy. Regarding the exception: I agree. How about RuntimeError?
Hide
Paul Price added a comment -

Sounds good.

Show
Paul Price added a comment - Sounds good.
Hide
Russell Owen added a comment - - edited

Do you mind if I change other TaskErrors to RuntimeError? I think all of them would benefit from a traceback.

Show
Russell Owen added a comment - - edited Do you mind if I change other TaskErrors to RuntimeError? I think all of them would benefit from a traceback.
Hide
Paul Price added a comment -

I think that's a good idea.

Show
Paul Price added a comment - I think that's a good idea.
Hide
Russell Owen added a comment -

I merged this to master. However, I made one minor mistake: I changed TaskError to RuntimeError on DM-5014 instead of this ticket. I will be merging that shortly, so I think no harm done. Thanks again for catching that. I was definitely over-using TaskError in the new tasks.

Show
Russell Owen added a comment - I merged this to master. However, I made one minor mistake: I changed TaskError to RuntimeError on DM-5014 instead of this ticket. I will be merging that shortly, so I think no harm done. Thanks again for catching that. I was definitely over-using TaskError in the new tasks.
 Resolution Done [ 10000 ] Status Reviewed [ 10101 ] Done [ 10002 ]
 Sprint Alert Production X16 [ 203 ]
 Epic Link DM-5053 [ 22713 ]
 Labels SciencePipelines

## People

• Assignee:
Russell Owen
Reporter:
Russell Owen
Reviewers:
Paul Price
Watchers:
Paul Price, Russell Owen