feat: Add support for optional namespace parameter in incoming webhook
#2290
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 enhances the incoming webhook functionality by introducing an optional Highlights
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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds support for an optional namespace parameter for incoming webhooks, which is a great feature for disambiguating repositories with conflicting names. The refactoring to introduce an incomingPayload struct and the performance improvement in GetRepoByName by using a field selector are excellent changes. The documentation has also been updated clearly.
I have a couple of suggestions for improvement. First, the error handling for incoming webhooks doesn't cover all cases, potentially leaving client requests without a response on certain errors. Second, the logic for parsing webhook parameters could lead to confusing behavior by implicitly mixing parameters from the URL query and the JSON body. My review includes suggestions to address these points.
| if errors.Is(err, errMissingFields) { | ||
| l.writeResponse(response, http.StatusBadRequest, err.Error()) | ||
| } | ||
| l.logger.Errorf("error processing incoming webhook: %v", err) | ||
| return |
There was a problem hiding this comment.
The error handling here is improved, but it still doesn't handle all error cases correctly. If l.detectIncoming returns an error that is not errDeprecatedRequestMode or errMissingFields, the error is logged but no HTTP response is sent to the client. This will likely result in the client request timing out or receiving an empty response, which is not ideal.
You should handle all other errors by sending an appropriate HTTP status code, for example, http.StatusInternalServerError for unexpected errors.
if errors.Is(err, errMissingFields) {
l.writeResponse(response, http.StatusBadRequest, err.Error())
} else {
l.writeResponse(response, http.StatusInternalServerError, "Error processing incoming webhook")
}
l.logger.Errorf("error processing incoming webhook: %v", err)
return
There was a problem hiding this comment.
We don't come back with custom HTTP Status code currently, it's not a bad idea, but this hasn't been requested yet, so we may skip it
When a Repository CR does not have a unique name in the cluster, a user must disambiguate the desired Repository by specifying a Namespace. The namespace is optional by default, but if the matcher finds more than one Repository of the same name then PaC will respond with the code 400 and indicate that the "namespace" parameter is required due to the ambiguity.
aThorp96
force-pushed
the
srvkp-5837-incoming-webhook-repo-conflict
branch
from
60619ad to
66f9939
Compare
| if errors.Is(err, errDeprecatedRequestMode) { | ||
| // TODO: change this to a request failure once the deprecation is removed | ||
| // In this specific case the error can be non-nil while the rest of the return values are valid | ||
| response.Header().Add("Deprecation", "true") | ||
| } else { |
There was a problem hiding this comment.
Note: this is new behavior and not particularly related to the principal change.
There was a problem hiding this comment.
i think we should use a X- prefixed header for that?
| if errors.Is(err, errMissingFields) { | ||
| l.writeResponse(response, http.StatusBadRequest, err.Error()) | ||
| } |
There was a problem hiding this comment.
The idea here being: If the required fields are missing then an errMissingFields is returned. If an optional field becomes required at runtime, the handler just cares that there was a missing field so an errMissingFields is also returned.
| } | ||
|
|
||
| repo, err := matcher.GetRepo(ctx, l.run, repository) | ||
| repo, err := matcher.GetRepoByName(ctx, l.run, payload.RepoName, payload.Namespace) |
There was a problem hiding this comment.
If no namespace is provided, the zero value "" acts as a wildcard in GetRepoByName's kubeapi request
| var errDeprecatedRequestMode = errors.New("requesting with secret name in query parameters is deprecated and will not be supported in future versions") | ||
|
|
||
| var errMissingFields = errors.New("missing required fields") | ||
|
|
||
| func errMissingSpecificFields(fields []string) error { | ||
| return fmt.Errorf("%w: %s", errMissingFields, fields) | ||
| } | ||
|
|
||
| type incomingPayload struct { | ||
| legacyMode bool // indicates the request was made using the deprecated queryparams method | ||
|
|
||
| RepoName string `json:"repository"` | ||
| Namespace string `json:"namespace,omitempty"` // Optional unless Repository name is not unique | ||
| Branch string `json:"branch"` | ||
| PipelineRun string `json:"pipelinerun"` | ||
| Secret string `json:"secret"` | ||
| Params map[string]any `json:"params"` | ||
| } |
There was a problem hiding this comment.
@aThorp96 I see a lot done for deprecation, can we have it in separate commit for better tracking?
There was a problem hiding this comment.
Yeah i agree i think we can add the feature in one PR and refactoring and depreactioin in the other
There was a problem hiding this comment.
Yeah I can factor that into a separate PR.
3 participants
📝 Description of the Change
Add support for optional
namespaceparameter in incoming webhookWhen a Repository CR does not have a unique name in the cluster, a user must disambiguate the desired Repository by specifying a Namespace. The namespace is optional by default, but if the matcher finds more than one Repository of the same name then PaC will respond with the code 400 and indicate that the "namespace" parameter is required due to the ambiguity.
👨🏻 Linked Jira
https://issues.redhat.com/browse/SRVKP-5837
🔗 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 [email protected]
Co-authored-by: ChatGPT [email protected]
Co-authored-by: Claude [email protected]
Co-authored-by: Cursor [email protected]
Co-authored-by: Copilot [email protected]
**💡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.