refactor(providers): constantize provider StatusOpt conclusion options#2384
Conversation
Summary of ChangesHello @aThorp96, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refactors the handling of status conclusions across the application's provider integrations. By introducing a dedicated Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
| ConclusionSuccess Conclusion = "success" | ||
| ) | ||
|
|
||
| //nolint:revive |
There was a problem hiding this comment.
I opted (no pun intended) to keep the existing struct's name StatusOpts even though the linter warned about the name being status.StatusOpts but I have no qualms with renaming to status.Opts, status.UpdateOpts, or something.
There was a problem hiding this comment.
Code Review
This is a great refactoring that introduces constants for StatusOpts.Conclusion values, improving type safety and maintainability across the providers. The changes are well-applied throughout the codebase. I've found a few places where new conclusion constants are not fully handled in provider-specific implementations, leading to // TODO comments or empty switch cases. I've provided suggestions to complete the implementation for these cases. I also have a minor suggestion to improve code readability in one of the pointer conversions.
| default: | ||
| } |
There was a problem hiding this comment.
The default case in this switch is empty. This means that several conclusion types (completed, skipped, cancelled) will be passed directly to gitea.StatusState, which may not be valid Gitea statuses. These should be explicitly mapped to valid Gitea statuses (e.g., pending, success, error, failure, warning).
For example:
ConclusionCompletedshould probably map togitea.StatusSuccess.ConclusionSkippedcould map togitea.StatusWarning.ConclusionCancelledcould map togitea.StatusFailure.
case providerstatus.ConclusionCompleted:
state = gitea.StatusSuccess
case providerstatus.ConclusionSkipped:
state = gitea.StatusWarning
case providerstatus.ConclusionCancelled:
state = gitea.StatusFailure
default:
// This will handle success, failure, pending which are already valid gitea.StatusState
}| default: | ||
| } |
There was a problem hiding this comment.
The default case in this switch is empty. This means that several conclusion types (completed, skipped, cancelled) will be passed directly to the GitHub API, which are not valid statuses. These should be explicitly mapped to valid GitHub statuses (error, failure, pending, success).
For example:
ConclusionCompletedandConclusionSkippedcould be mapped toConclusionSuccess.ConclusionCancelledcould be mapped toConclusionFailure.
| default: | |
| } | |
| case providerstatus.ConclusionCompleted, providerstatus.ConclusionSkipped: | |
| status.Conclusion = providerstatus.ConclusionSuccess | |
| case providerstatus.ConclusionCancelled: | |
| status.Conclusion = providerstatus.ConclusionFailure | |
| default: | |
| } |
There was a problem hiding this comment.
can you double check @aThorp96 this is the case? this seems relevant...
There was a problem hiding this comment.
I think this comment may be correct, but there is no change in behavior in this PR. Can these be fixed in a followup PR?
| case status.ConclusionCancelled: | ||
| // TODO | ||
| } |
There was a problem hiding this comment.
| // TODO: Should this be scm.StateRunning? | ||
| state = scm.StatePending |
There was a problem hiding this comment.
The TODO comment questions whether scm.StateRunning should be used. Since the status title is "CI has started", scm.StateRunning seems more appropriate than scm.StatePending. go-scm defines StateRunning as "The build is running.".
| // TODO: Should this be scm.StateRunning? | |
| state = scm.StatePending | |
| state = scm.StateRunning |
| case status.ConclusionCancelled: | ||
| // TODO | ||
| } |
| case providerstatus.ConclusionCancelled, providerstatus.ConclusionCompleted, providerstatus.ConclusionSkipped: | ||
| } |
There was a problem hiding this comment.
This new case for cancelled, completed, and skipped conclusions is empty. This means statusOpts.Title and statusOpts.Summary won't be set for these cases, which could lead to confusing or missing information in the status updates. These cases should be handled explicitly.
For example:
ConclusionCompletedcould be treated likeConclusionSuccess.ConclusionCancelledcould be aFailureorWarning.ConclusionSkippedcould be aWarning.
case providerstatus.ConclusionCompleted:
statusOpts.Title = "Completed"
statusOpts.Summary = "has <b>completed</b>."
case providerstatus.ConclusionCancelled:
statusOpts.Title = "Cancelled"
statusOpts.Summary = "has been <b>cancelled</b>."
case providerstatus.ConclusionSkipped:
statusOpts.Title = "Skipped"
statusOpts.Summary = "has been <b>skipped</b>."
}| case providerstatus.ConclusionCompleted, providerstatus.ConclusionSkipped: | ||
| } |
There was a problem hiding this comment.
This new case for completed and skipped conclusions is empty. This means statusOpts.Title and statusOpts.Summary won't be set for these cases, which could lead to confusing or missing information in the status updates. These cases should be handled explicitly.
For example:
ConclusionCompletedcould have a title "Completed".ConclusionSkippedcould have a title "Skipped".
| case providerstatus.ConclusionCompleted, providerstatus.ConclusionSkipped: | |
| } | |
| case providerstatus.ConclusionCompleted: | |
| statusOpts.Title = "Completed" | |
| statusOpts.Summary = "has <b>completed</b>." | |
| case providerstatus.ConclusionSkipped: | |
| statusOpts.Title = "Skipped" | |
| statusOpts.Summary = "has been <b>skipped</b>." | |
| } |
ce689be to
d2ef3de
Compare
🔍 PR Lint Feedback
|
80ddccb to
8d2dff2
Compare
|
Note on the Gemini feedback: most of the Gemini feedback is requesting I enumerate switch statements and/or have logic for some currently-noop cases (e.g. Gitea doesn't handle Cancelled, Completed, or Skipped). A downside of using a pseudo-enum for these states is that the linter requires |
theakshaypant
left a comment
There was a problem hiding this comment.
LGTM, I would still like to wait for the CI fix to run e2e on the PR before merging it.
|
can you double check the default case if they are being passed directly to the provider status ? i am uncertain it's the case but i haven't dug down into the code |
c7a9a38 to
c77cb57
Compare
|
@aThorp96 again there are many conflicts 🥲 |
The Conclusion field of StatusOpts has a discrete number of supported values. These being typed as strings has introduced bugs in the past and makes adding new values difficult. Defining the supported values as constant enums improves both the safety of the values and the ease of working with them.
1befc05 to
dd7cfa8
Compare
|
Only did a rebase. Will address the comments in a separate commit next |
|
Thank you @theakshaypant 🙏🏻 |
📝 Description of the Change
The Conclusion field of StatusOpts has a discrete number of supported values. These being typed as strings has introduced bugs in the past and makes adding new values difficult. Defining the supported values as constant enums improves both the safety of the values and the ease of working with them.
👨🏻 Linked Jira
None
🔗 Linked GitHub Issue
Fixes #
🚀 Type of Change
fix:)feat:)feat!:,fix!:)docs:)chore:)refactor:)enhance:)deps:)🧪 Testing Strategy
🤖 AI Assistance
If you have used AI assistance, please provide the following details:
Which LLM was used?
Extent of AI Assistance:
Important
If the majority of the code in this PR was generated by an AI, please add a
Co-authored-bytrailer to your commit message.For example:
Co-authored-by: Gemini gemini@google.com
Co-authored-by: ChatGPT noreply@chatgpt.com
Co-authored-by: Claude noreply@anthropic.com
Co-authored-by: Cursor noreply@cursor.com
Co-authored-by: Copilot Copilot@users.noreply.github.com
**💡You can use the script
./hack/add-llm-coauthor.shto automatically addthese co-author trailers to your commits.
✅ Submitter Checklist
fix:,feat:) matches the "Type of Change" I selected above.make testandmake lintlocally to check for and fix anyissues. For an efficient workflow, I have considered installing
pre-commit and running
pre-commit installtoautomate these checks.