Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upAdd commands for managing GitHub Releases #1552
Conversation
We install an HTTP middleware that adds the "Authorization" header on every HTTP request. However, our asset download process might redirect to a 3rd-party host (Amazon S3) and we want to allow those requests but not require that they are authenticated. Furthermore, we need the ability to specify the `Accept` request header without it being overwritten by middleware, so now middleware only adds headers that are not present in a request.
|
looking forward to it |
|
I'm so excited for this one, it looks great! Had one nit from going through things: Should we autocomplete the title from the tag? |
bigcook12345 left a comment
|
So it works now..? |
@ampinsk We can consider this, but the release title is technically optional on the server and, if left blank, will inherit the title of the git tag (if any) or the subject of the git commit that the tag points to. If people are actively counting on this behavior by intentionally leaving the title blank, then by pre-filling the title to match the tag name, we effectively disallow that fallback on the client. I'm on the fence about what the ideal behavior would be, but for the sake of predictability and parity with platform behavior, I'm currently leaning to allowing blank titles. |
|
@mislav was definitely just a nit, totally fine with dropping it! |
|
this was a lot to review at once >_> i didn't see any blockers to merging, but had some light requests here and there so i'm marking request changes. overall i found the new commands to be very comprehensible and well organized, thanks! |
| @@ -45,7 +45,9 @@ func NewClientFromHTTP(httpClient *http.Client) *Client { | |||
| func AddHeader(name, value string) ClientOption { | |||
| return func(tr http.RoundTripper) http.RoundTripper { | |||
| return &funcTripper{roundTrip: func(req *http.Request) (*http.Response, error) { | |||
| req.Header.Add(name, value) | |||
| if len(req.Header.Values(name)) == 0 { | |||
This comment has been minimized.
This comment has been minimized.
vilmibm
Sep 1, 2020
Member
why forbid overriding, here? I would expect AddHeader to let me replace an existing header with a new value.
| ResponseHeader: logTraffic, | ||
| ResponseBody: logTraffic, | ||
| Formatters: []httpretty.Formatter{&httpretty.JSONFormatter{}}, | ||
| MaxResponseBody: 10000, |
This comment has been minimized.
This comment has been minimized.
vilmibm
Sep 1, 2020
Member
is this intentional? I do this a lot locally so I certainly wouldn't mind it being in trunk.
This comment has been minimized.
This comment has been minimized.
mislav
Sep 2, 2020
Author
Member
Good spot! Yes, it's intentional— I realized that I wasn't able to debug large response payloads without this.
| @@ -45,7 +45,9 @@ func NewClientFromHTTP(httpClient *http.Client) *Client { | |||
| func AddHeader(name, value string) ClientOption { | |||
| return func(tr http.RoundTripper) http.RoundTripper { | |||
| return &funcTripper{roundTrip: func(req *http.Request) (*http.Response, error) { | |||
| req.Header.Add(name, value) | |||
| if req.Header.Get(name) == "" { | |||
This comment has been minimized.
This comment has been minimized.
vilmibm
Sep 1, 2020
Member
why prevent overriding, here? I'd think that repeated calls to AddHeader would update a header value, not silently fail to update.
This comment has been minimized.
This comment has been minimized.
mislav
Sep 2, 2020
Author
Member
It's a good question! The short answer is that I had to add the guard to allow file downloads. Without this change, a file download request would set an Accept: application/octet-stream, but additional Accept values would get added by our request middleware via AddHeader and AddHeaderFunc, and thus JSON payload would get returned and the download sabotaged.
When I then thought about our AddHeader and AddHeaderFunc utilities, I realized that they are more in the service of adding default request headers (e.g. default Accept, default User-Agent, default Authorization), but if a request already contained any of those headers explicitly, there would be no reason to apply the default.
You have a good point that maybe we should change the verb from "Add" to something else that reflects the current semantics better?
This comment has been minimized.
This comment has been minimized.
vilmibm
Sep 8, 2020
Member
ah, that makes sense -- in that case yeah I think we can rename from AddHeader to something like SetHeaderDefault or similar
| } | ||
|
|
||
| func publishRelease(httpClient *http.Client, releaseURL string) error { | ||
| req, err := http.NewRequest("PATCH", releaseURL, bytes.NewBufferString(`{"draft":false}`)) |
This comment has been minimized.
This comment has been minimized.
vilmibm
Sep 1, 2020
Member
what is your motivation for moving away from client.REST? I don't have an issue with it, just curious.
This comment has been minimized.
This comment has been minimized.
mislav
Sep 2, 2020
Author
Member
First and foremost is because REST requires me to instantiate an api.Client, which is an outdated pattern that I would like us to move away from since it acted as a wrapper around http.Client, but did not provide any benefits.
An additional reason was that I was hoping to see how comfortable I was letting specific commands control most aspects of how they process HTTP requests. This is with the goal of decreasing inter-package dependencies. I'm satisfied by this approach, but I do believe that error handling should be shared between all commands, as well as authentication.
| }, | ||
| } | ||
|
|
||
| cmd.Flags().StringVarP(&opts.Destination, "dir", "C", ".", "The directory to download files into") |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
mislav
Sep 3, 2020
Author
Member
Good question! I guess I was biased towards -C because commands like tar, make, and git support it. Wget does -P, --directory-prefix. I will change to -D
| } | ||
|
|
||
| cmd := &cobra.Command{ | ||
| Use: "download <tag> [<pattern>]", |
This comment has been minimized.
This comment has been minimized.
vilmibm
Sep 1, 2020
Member
I think it's worth documenting what kind of pattern this can be; when I was testing this out I wasn't sure if it was glob, plain wildcard, or regular expression.
| var downloadError error | ||
| for i := 0; i < len(toDownload); i++ { | ||
| if err := <-results; err != nil { | ||
| downloadError = err |
This comment has been minimized.
This comment has been minimized.
vilmibm
Sep 1, 2020
Member
I wonder if it's worth a break here if we're not going to collect errors per-download. Otherwise we're discarding errors for all but the last download which is confusing.
This comment has been minimized.
This comment has been minimized.
mislav
Sep 3, 2020
Author
Member
Good thought, but we cannot break here since we need to invoke <-results as many times as there were download jobs. You're right that all but the latest error gets discarded. Right now I don't think that's such an issue, but if people notice this, we could collect all errors into one meta-error
| } | ||
|
|
||
| // FindDraftRelease interates over all releases in a repository until it finds one that matches tagName. | ||
| func FindDraftRelease(httpClient *http.Client, baseRepo ghrepo.Interface, tagName string) (*Release, error) { |
This comment has been minimized.
This comment has been minimized.
vilmibm
Sep 1, 2020
Member
does this exist because draft releases aren't included when using releases/tag/:foo? I'm confused how the Draft part of this function's name relates to what it actually does.
This comment has been minimized.
This comment has been minimized.
mislav
Sep 3, 2020
Author
Member
Yes, the API doesn't let us query draft releases by tag name (makes sense, since there can be multiple drafts referring to the same tag name), so I have to resort to listing all releases. You are right that this lookup returns any release that matches a tag, not necessarily draft. To stay true to the function name, I will add code that only scopes the lookup to draft releases.
| bold = ansi.ColorFunc("default+b") | ||
| ) | ||
|
|
||
| func NewColorScheme(enabled bool) *ColorScheme { |
This comment has been minimized.
This comment has been minimized.
vilmibm
Sep 1, 2020
Member
i like this approach to color! but please open a tech debt issue to port the whole codebase to it so we don't end up with competing color systems.
|
Two questions, is this path error due to the test mode or a bug? and is it ok to skip the title? It will create a Draft title more than once.
Questions:
|
that error looks related to a bad value set for |
|
@tierninho All great questions!
The same as without the
That's a good idea! I will add it.
We haven't talked about it, but a message like “6 files successfully uploaded” might be nice. I'll consider adding it!
Yes because a draft will usually get published, and when it does then the original "prerelease" bit will matter. The "draft" and "prerelease" bits are thus independent of each other. |
cyrilletuzi
commented
Sep 9, 2020
|
@mislav First, thank you for this feature! <3 But is there a way to use the latest created tag (for example It seems very important to me to really be usable in an automation process. For exemple:
vsce publish patch && git push && git push --tags
npm run build && npm version minor && npm publish dist && git push && git push --tags;In both cases, the tag creation is automated (by |
|
@cyrilletuzi That's an excellent feature request and something that I haven't considered; thank you! Would you mind opening a separate issue so we can discuss and track this? In the meantime, would it be an option for you to use the output of |
mislav commentedAug 19, 2020
•
edited
New commands:
gh release listgh release view <tag>gh release create <tag> [<files>...]gh release upload <tag> <files>...gh release download <tag> [<pattern>]Example output:
Example help:
TODO:
gh release uploadcommandgh release downloadcommandgh release deletecommandFixes #378