-
Notifications
You must be signed in to change notification settings - Fork 14
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add optional omit_target_url #160
Conversation
6e33f8f
to
31b21b0
Compare
@@ -54,6 +54,9 @@ func (sink GitHubCommitStatusSink) Send() error { | |||
"state", ghState, "owner", sink.Request.Source.Owner, | |||
"repo", sink.Request.Source.Repo, "git-ref", sink.GitRef, | |||
"context", context, "buildURL", buildURL, "description", description) | |||
if sink.Request.Source.OmitTargetURL { | |||
buildURL = "" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As the GH API expect a string or null, wouldn't it be cleaner to pass null instead?
(https://docs.github.com/en/rest/commits/statuses?apiVersion=2022-11-28#create-a-commit-status)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm also tempted to do this before logging to avoid a misleading buildURL
showing up in the logs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm also tempted to do this before logging to avoid a misleading
buildURL
showing up in the logs
That was my first iteration. Then I realized that the logging should still contain the buildURL, since the logging is not publicly accessible, and the goal of this PR is to omit the Concourse URL from a public GitHub PR page. Omitting also from the log would, in my understanding, make troubleshooting or auditing more difficult for no reason...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As the GH API expect a string or null, wouldn't it be cleaner to pass null instead? (https://docs.github.com/en/rest/commits/statuses?apiVersion=2022-11-28#create-a-commit-status)
The type of buildURL is a string, not a pointer to a string, so we cannot set it to nil. We could change the type to pointer to string, but that would bring pain everywhere else.
What we could do is to decorate the structure field TargetURL
with tag omitempty
Lines 83 to 89 in a55a812
// AddRequest is the JSON object sent to the API. | |
type AddRequest struct { | |
State string `json:"state"` | |
TargetURL string `json:"target_url"` | |
Description string `json:"description"` | |
Context string `json:"context"` | |
} |
like this:
TargetURL string `json:"target_url,omitempty"`
omitempty
will see an empty string (the value ""
) and will omit the field from the JSON object. This is not the same as setting the field to JSON null...
To summarize, I am tempted to say that if the GH API is happy with an empty string, that is good enough. But this discussion made me realize that I need to test this somehow.
UPDATE
Tested with empty ""
targetUrl. Works as expected: adds a status check according to the passed parameters. Link "Details", that normally points to the targetURL, in this case points back to the commit page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@odormond I am mentioning you here because I am going to merge this PR. I believe I addressed both your comments. If I missed something, ping me.
31b21b0
to
2743c08
Compare
While at it, did some cleanups and updated to Go 1.23
Closes #159.