Uploaded image for project: 'Data Management'
  1. Data Management
  2. DM-5406

Require fields listed in icSourceFieldsToCopy to be present

    Details

      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).

        Attachments

          Issue Links

            Activity

            rowen Russell Owen created issue -
            rowen Russell Owen made changes -
            Field Original Value New Value
            Link This issue is triggered by DM-5385 [ DM-5385 ]
            rowen Russell Owen made changes -
            Status To Do [ 10001 ] In Progress [ 3 ]
            Hide
            rowen 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
            rowen 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.
            rowen Russell Owen made changes -
            Reviewers Paul Price [ price ]
            Status In Progress [ 3 ] In Review [ 10004 ]
            Hide
            price 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
            price 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.
            price Paul Price made changes -
            Status In Review [ 10004 ] Reviewed [ 10101 ]
            Hide
            rowen 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
            rowen 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
            price Paul Price added a comment -

            Sounds good.

            Show
            price Paul Price added a comment - Sounds good.
            Hide
            rowen 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
            rowen 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
            price Paul Price added a comment -

            I think that's a good idea.

            Show
            price Paul Price added a comment - I think that's a good idea.
            Hide
            rowen 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
            rowen 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.
            rowen Russell Owen made changes -
            Resolution Done [ 10000 ]
            Status Reviewed [ 10101 ] Done [ 10002 ]
            rowen Russell Owen made changes -
            Sprint Alert Production X16 [ 203 ]
            rowen Russell Owen made changes -
            Epic Link DM-5053 [ 22713 ]
            rowen Russell Owen made changes -
            Labels SciencePipelines

              People

              • Assignee:
                rowen Russell Owen
                Reporter:
                rowen Russell Owen
                Reviewers:
                Paul Price
                Watchers:
                Paul Price, Russell Owen
              • Votes:
                0 Vote for this issue
                Watchers:
                2 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:

                  Summary Panel