⚡ Updated build run syncstatus#302
Conversation
abdheshnayak
commented
Mar 17, 2024
abdheshnayak
commented
- updated syncstatus for buildrun [ needs to test ]
- added option for build trigger
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @abdheshnayak - I've reviewed your changes and they look great!
General suggestions:
- Ensure new parameters introduced in methods are fully utilized and documented to avoid confusion.
- Review the security implications of the newly added 'seed' parameter in the context of generating unique keys.
- Consider providing more explicit error handling for parsing operations to enhance debugging capabilities.
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟡 Security: 1 issue found
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Docstrings: all looks good
Thanks for using Sourcery. We offer it for free for open source projects and would be very grateful if you could help us grow. If you like it, would you consider sharing Sourcery on your favourite social media? ✨
| func (d *Impl) parseRecordVersionFromAnnotations(annotations map[string]string) (int, error) { | ||
| annotatedVersion, ok := annotations[constants.RecordVersionKey] | ||
| if !ok { | ||
| return 0, errors.Newf("no annotation with record version key (%s), found on the resource", constants.RecordVersionKey) | ||
| } | ||
|
|
||
| annVersion, err := strconv.ParseInt(annotatedVersion, 10, 32) | ||
| if err != nil { | ||
| return 0, errors.NewE(err) | ||
| } | ||
|
|
||
| return int(annVersion), nil | ||
| } |
There was a problem hiding this comment.
suggestion (code_refinement): Consider handling potential strconv.ParseInt errors more explicitly.
While the error from strconv.ParseInt is wrapped using errors.NewE, providing a more specific error message could help in debugging issues related to parsing the annotated version.
| } | ||
|
|
||
| return int(annVersion), nil | ||
| } |
There was a problem hiding this comment.
question (code_clarification): Ensure proper handling of the new 'status' parameter.
The 'status' parameter introduced in OnBuildRunUpdateMessage does not seem to be used within the function. If it's intended for future use, consider documenting its purpose or implementing its usage if applicable.
| } | ||
|
|
||
| return int(annVersion), nil | ||
| } |
There was a problem hiding this comment.
🚨 question (security): Validate the 'seed' parameter usage in CreateBuildRun.
The addition of the 'seed' parameter in CreateBuildRun is interesting. Ensure that its usage in generating a unique key is secure and meets the intended purpose, especially in the context of potential replay or collision attacks.
⚡ Updated build run syncstatus
1 participant
