Repository navigation
Conversation
fba73a1 to
040b153
Compare
fstagni
left a comment
fstagni
left a comment
There was a problem hiding this comment.
There are also no tests added.
| for paramName, paramValue in (customParameters or {}).items(): | ||
| res = self.transClient.setTransformationParameter(transID, paramName, paramValue) | ||
| if not res["OK"]: | ||
| deleteRes = self.transClient.deleteTransformation(transID) |
There was a problem hiding this comment.
Can you explain your reasoning here? As I read it, if you fail to set a parameter you remove the whole transformation.
There was a problem hiding this comment.
It is also worth logging which parameter failed.
There was a problem hiding this comment.
You are right, there is no reason to delete the transformation here. I'll remove it.
There was a problem hiding this comment.
You are right, there is no reason to delete the transformation here. I'll remove it.
Thinking a little bit further, the original reason for which I added the deletion here it's to not leave partially configured transformations.
Does the logic make sense for you?
| return S_OK() | ||
|
|
||
| def addTransformationStep(self, stepID, prodID): | ||
| def addTransformationStep(self, stepID, prodID, customParameters=None): |
There was a problem hiding this comment.
If one of these parameter is "Status", or "Body", will it overwrite the status or body of the transformation? Is that the scope?
There was a problem hiding this comment.
In principle Body should not be overwritten as it's in the _STEP_ATTRIBUTES , see:
but it's true the Status would be.
I guess that I should also add Status to the list of _STEP_ATTRIBUTES and also all other Fields in the Transformations table, right?
There was a problem hiding this comment.
In principle
Bodyshould not be overwritten as it's in the_STEP_ATTRIBUTES, see:but it's true the
Statuswould be.I guess that I should also add
Statusto the list of_STEP_ATTRIBUTESand also all other Fields in the Transformations table, right?
I believe so.
You mean unit tests or updating the existing integration tests for the Production System? |
b487083 to
a9f8e6d
Compare
|
I've added some unit tests. I see however that some CI tests are failing but it does not seem related to my changes. |
Prob fixed by #8876 |
BEGINRELEASENOTES
NEW: add the possibility to define custom parameters for the production steps
ENDRELEASENOTES