Remove obsolete apps manifest preview custom header #1858
Conversation
google-cla
bot
added
the
cla: yes
chmouel
force-pushed
the
apps-manifest-mediaTypeV3
branch
from
e9da9b3 to
a7a6816
Compare
github/apps_manifest.go
Outdated
| @@ -41,7 +37,7 @@ func (s *AppsService) CompleteAppManifest(ctx context.Context, code string) (*Ap | |||
| if err != nil { | |||
| return nil, nil, err | |||
| } | |||
| req.Header.Set("Accept", mediaTypeAppManifestPreview) | |||
| req.Header.Set("Accept", mediaTypeV3) | |||
There was a problem hiding this comment.
Please remove this line, as this header is added to all requests here:
https://github.com/google/go-github/blob/master/github/github.go#L383
chmouel
force-pushed
the
apps-manifest-mediaTypeV3
branch
from
a7a6816 to
3037c66
Compare
Codecov Report
@@ Coverage Diff @@
## master #1858 +/- ##
==========================================
- Coverage 97.64% 97.64% -0.01%
==========================================
Files 105 105
Lines 6764 6763 -1
==========================================
- Hits 6605 6604 -1
Misses 86 86
Partials 73 73
Continue to review full report at Codecov.
|
github/apps_manifest_test.go
Outdated
| @@ -30,7 +30,7 @@ func TestGetConfig(t *testing.T) { | |||
|
|
|||
| mux.HandleFunc("/app-manifests/code/conversions", func(w http.ResponseWriter, r *http.Request) { | |||
| testMethod(t, r, "POST") | |||
| testHeader(t, r, "Accept", mediaTypeAppManifestPreview) | |||
| testHeader(t, r, "Accept", mediaTypeV3) | |||
There was a problem hiding this comment.
Let's please also remove this line for conformity with the rest of the repo.
Apps manifest are now out of preview and uses mediaTypeV3: https://docs.github.com/en/rest/reference/apps#create-a-github-app-from-a-manifest
chmouel
force-pushed
the
apps-manifest-mediaTypeV3
branch
from
3037c66 to
7f40182
Compare
|
For future PRs, please do not use force push in this repo. See CONTRIBUTING.md for details. Thanks. |
|
Thank you, @wesleimp ! |
|
@bluekeyes - it just dawned on me... do you need to keep this preview header around for GitHub Enterprise before I merge this? |
|
@gmlewis thanks for checking, I think this is probably fine. The GitHub docs for the supported enterprise versions don't reference this header (not that it's a great guarantee the header is unused) and I've personally never used this endpoint with the instances I maintain. |
gmlewis
changed the title
Apps manifest are now out of preview and uses mediaTypeV3:
https://docs.github.com/en/rest/reference/apps#create-a-github-app-from-a-manifest