Add a predefined error code for build CRD as the prefix of error message #483
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
There was a problem hiding this comment.
I think it would also be important to document error codes. I suggest a new section at the end of build.md. This helps the consumers of our controller who write a client (like a UI) for it, to get a list of possible error situations that they could face and would need to present to their users.
| NoClusterBuildStrategyFound string = "010: no ClusterBuildStrategies found" | ||
| ClusterBuildStrategyDoesNotExist string = "011: clusterBuildStrategy %s does not exist" | ||
| // validate runtime | ||
| RuntimeCanNotBeEmpty string = "012: the property 'spec.runtime.paths' must not be empty" |
There was a problem hiding this comment.
I would rename the constant to: RuntimePathsCanNotBeEmpty.
Good idea, I will add this part, thanks! |
|
Also, integration-tests are failing with this pr, pls take a look |
|
@qu1queee @SaschaSchwarze0 @zhangtbj |
There was a problem hiding this comment.
@xiujuan95 adding my second round of request for changes, pls take a look. I think what is missing is to enhance test to validate that for each type of validation, the Status.Reason contains the right code, and also to ensure existing codes are not deleted. I also added a proposal on not using the prependen "B" for the code, but rather something else. We are close to finish this one, thanks
|
I left a comment on the parent issue #463. I question the usability of numeric error codes when adding Conditions presents a viable alternative. |
|
@xiujuan95 looks good to me, the only last requirement from my side is to squash your commits. Either three(code change, docs and test), or a single one. Also the whole idea of this PR is to not modify error codes in the future(after this goes into a build release), only the error message can be changed over time. @SaschaSchwarze0 probably you should take another review here. Note: We need to distinguish between error codes for secrets, we currently are adding |
@qu1queee Done! |
There was a problem hiding this comment.
I´m approving, but not yet merging. I would like someone from RedHat to give an approval also. @sbose78 @adambkaplan
|
Thank you for the good work on this @xiujuan95 . Apologies for not getting to this on time. @adambkaplan wouldn't be available to have a detailed discussion on this, this week. I'll provide my opinion on this: I am not naturally aligned to use error codes to be very honest, I happy to discuss. Is this any precedence on this approach I could take a look at ? |
|
Closed based on latest comments in #463 |
issue shipwright-io#463 . Keep in mind this would be the second attempt to improve on this topic, we previously tried it with: - shipwright-io#483 Signed-off-by: Zoe <xigxjn@cn.ibm.com>
issue shipwright-io#463 . Keep in mind this would be the second attempt to improve on this topic, we previously tried it with: - shipwright-io#483 Signed-off-by: Zoe <xigxjn@cn.ibm.com>
issue shipwright-io#463 . Keep in mind this would be the second attempt to improve on this topic, we previously tried it with: - shipwright-io#483 Signed-off-by: Zoe <xigxjn@cn.ibm.com>
issue shipwright-io#463 . Keep in mind this would be the second attempt to improve on this topic, we previously tried it with: - shipwright-io#483 Signed-off-by: Zoe <xigxjn@cn.ibm.com>
issue shipwright-io#463 . Keep in mind this would be the second attempt to improve on this topic, we previously tried it with: - shipwright-io#483 Signed-off-by: Zoe <xigxjn@cn.ibm.com>
Fix issue: #463
This PR add a predefined error code as the prefix of error message, so that the third party can get the message what they want against this error code. These error codes will never be changed. So And the third party will not be affected by error message's contents.
Now, I use three numbers as the prefix, and begin with
001. Also I extract all error message and put them at the beginning of codes. In the future, if we want to add more message, then we can append them in order.I don't add extra functions, so I think add test cases are not necessary.