Use metadata to reconcile go-github with GitHub's OpenAPI descriptions #2919

WillAbides · 2023-09-09T14:08:08Z

TODO before submitting for review:

Fix line-end issue on Windows
Update CONTRIBUTING.md
Add copyright notices to new files

This PR introduces a file called openapi_operations.yaml that contains details of GitHub's endpoints as described in https://github.com/github/rest-api-description. When combined with the //meta:operation directive, this allows us to automatically keep documentation links up to date, but more importantly it allows us to track endpoints as they are added and removed.

The change set is huge, but it is primarily made up of openapi_operations.yaml itself and changes to godoc.

"tools/cmd/metadata" is a tool to manage metadata.yaml. I'll paste its help output here for convenience:

Usage: metadata <command>

Flags:
  -h, --help               Show context-sensitive help.
  -C, --working-dir="."    Working directory. Should be the root of the go-github repository.

Commands:
  update-openapi
    Update openapi_operations.yaml from OpenAPI descriptions in github.com/github/rest-api-description at the given git ref.

  update-go
    Update go source code to be consistent with openapi_operations.yaml.
      - Adds and updates "// GitHub API docs:" comments for service methods.
      - Updates "//meta:operation" comments to use canonical operation names.
      - Updates formatting of "//meta:operation" comments to make sure there isn't a space between the "//" and the "meta".
      - Formats modified files with the equivalent of "go fmt".

  format
    Format whitespace in openapi_operations.yaml and sort its operations.

  unused
    List operations in openapi_operations.yaml that aren't used by any service methods.

Run "metadata <command> --help" for more information on a command.

I'm trying to limit the scope of this PR because its line count is already pretty high. I will make a follow-up PR with a daily scheduled workflow that runs "update-openapi" and "update-go" and creates a PR if there are changes.

codecov · 2023-09-09T14:10:55Z

Codecov Report

Merging #2919 (4a065a9) into master (5b34ea7) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #2919   +/-   ##
=======================================
  Coverage   98.11%   98.11%           
=======================================
  Files         149      149           
  Lines       12796    12796           
=======================================
  Hits        12555    12555           
  Misses        166      166           
  Partials       75       75

Files	Coverage Δ
github/actions_artifacts.go	`95.71% <ø> (ø)`
github/actions_cache.go	`100.00% <ø> (ø)`
github/actions_oidc.go	`100.00% <ø> (ø)`
github/actions_permissions_enterprise.go	`100.00% <ø> (ø)`
github/actions_permissions_orgs.go	`100.00% <ø> (ø)`
github/actions_required_workflows.go	`100.00% <ø> (ø)`
github/actions_runner_groups.go	`100.00% <ø> (ø)`
github/actions_runners.go	`100.00% <ø> (ø)`
github/actions_secrets.go	`100.00% <ø> (ø)`
github/actions_variables.go	`100.00% <ø> (ø)`
... and 133 more

gmlewis

I'll be OOO for one week, but before I go, I just wanted to point out that some of the new URLs have double slashes next to each other, e.g. https://docs.github.com/enterprise-cloud@latest//rest/actions/cache#get-github-actions-cache-usage-for-an-enterprise.

Also, in cases where URLs have been completely removed, it might be nice to point
to explanations as to why they are being removed, such as:

Have a great week, and thanks for doing this, @WillAbides !

WillAbides · 2023-09-09T20:00:41Z

@gmlewis enjoy your time away.

Those double slashed URLs are the same way in the OpenAPI description. They work as-is, but I wouldn't mind normalizing the path for aesthetic reasons.

That's a good point about adding explanations to the ones that lost their doc links. I'll go through those and add some notes.

WillAbides · 2023-09-10T15:57:45Z

I updated update-urls to only modify links that don't resolve to the same docs. This makes the change set for this PR more manageable.

# Conflicts: # .github/workflows/linter.yml # .github/workflows/tests.yml # github/misc.go # github/repos.go # github/repos_contents.go # update-urls/go.mod

gmlewis

Wow, @WillAbides - very nice.
I'm looking forward to using it.
It looks like the PR is still a "Draft" but looks pretty complete to me. What's missing?
Maybe more instructions for maintainers? It looks pretty automated, though.

WillAbides · 2023-10-16T15:03:52Z

@gmlewis You're right about instructions. After I started to work on CONTRIBUTING.md I realized that this adds complexity for new contributors by asking them to edit a large yaml file. I also suspect that most new contributors don't do much more than skim CONTRIBUTING.md before making their first PR. I know I'm often guilty of that for new projects. A typical new contribution would be:

code the change
create the PR
wait for CI to be approved
find out you need to update metadata.yaml

I gave it a little more thought and came up with a change I think will help. People tend to copy and adapt existing code. We can move the operation mapping to method a directive comment so that new contributors will see it on other methods as they write their code.

The code comment for RepositoriesService.Get would become:

// Get fetches a repository.
//
// GitHub API docs: https://docs.github.com/en/rest/repos/repos#get-a-repository
//
//meta:operation GET /repos/{owner}/{repo}

//meta:operation ... is a directive comment so godoc ignores it and outputs the same as before.

I started coding this up thinking it would add complexity, but it actually simplifies the tooling code quite a bit.

What do you think of moving the mapping to a directive comment?

gmlewis · 2023-10-16T15:31:38Z

I think that is a great idea, @WillAbides.
I notice that most contributors, myself included, start with an existing working example, then modify it for the task at hand.

This is super obvious when I ask for a change in a PR and their reply is "it was done this way over here". Sometimes my response is, fine, keep it, but sometimes I say "can you please fix it over there too?" 😄

So having this with each method will probably make the maintenance burden much lower moving forward, as it will be obvious that it is needed for each method. I like it.

WillAbides · 2023-10-16T16:54:56Z

I'm glad you like that idea. I have it mostly working, but it might be a few days before I have time to finish it up.

# Conflicts: # update-urls/go.mod # update-urls/go.sum

gmlewis

This looks great, @WillAbides !
Just a few minor tweaks, please, then we'll be ready for a second LGTM+Approval before merging.

CONTRIBUTING.md

gmlewis · 2023-10-19T18:51:50Z

CONTRIBUTING.md

+  in `openapi_commit`. `update-openapi --validate` is called
+  by `script/lint.sh`.
+
+- `update-go` - updates go files with documentation urls and formats comments.


"go" => "Go" or maybe ".go" here?
Also, "urls" => "URLs".

CONTRIBUTING.md

gmlewis · 2023-10-19T19:05:18Z

tools/metadata/openapi.go

+			major, _ := strconv.Atoi(m[pattern.SubexpIndex("major")])
+			minor, _ := strconv.Atoi(m[pattern.SubexpIndex("minor")])


Do we really not care here if the major and/or minor versions are not supplied?
If so, could you please add a comment before line 132 stating that we don't care?

I updated it to return errors

tools/metadata/metadata_test.go

tools/metadata/main.go

Co-authored-by: Glenn Lewis <[email protected]>

gmlewis

Thank you, @WillAbides !
LGTM.

Awaiting second LGTM+Approval from any other contributor to this repo before merging.

# Conflicts: # github/github.go

vandanrohatgi

I'm not sure if my approval helps here (won't be offended if we decide to wait for another approval), but I have gone through some the changes and LGTM.
Hoping this moves things along a bit.

# Conflicts: # github/rate_limit.go

gmlewis · 2023-11-03T14:26:19Z

Thank you, @vandanrohatgi and @valbeat !
Merging.

WillAbides added 10 commits September 8, 2023 14:44

update doc urls

3083413

use metadata.yaml to reconcile source with OpenAPI descriptions

11baddf

trigger

462108e

minor refactor

67d827b

go fmt

73e2220

update tools module

d6360bd

revert script dir

7f4b553

restore gen-accessors.go and gen-stringify-test.go

4aa1c11

restore .gitignore

a1ba628

go 1.20

7b7df59

gmlewis reviewed Sep 9, 2023

View reviewed changes

WillAbides added 6 commits September 9, 2023 16:35

update tooling and undocumented methods

fa02527

leave existing doc links in place

d855286

restore github dir

43c3216

update regex

ae5fd84

update-urls

000231c

include enterprise-server version in doc links

e21c51b

WillAbides added 10 commits September 10, 2023 11:00

add copyright notice

1bf8fd6

use ast in update-urls

0137f71

use ast in getServiceMethodsFromFile

0f657ae

go fmt

dfb3c8c

Use enterprise-cloud links

9a85b69

Merge branch 'master' into meta11

63190e9

# Conflicts: # .github/workflows/linter.yml # .github/workflows/tests.yml # github/misc.go # github/repos.go # github/repos_contents.go # update-urls/go.mod

refactor

b0ad415

fix comment

fcd94b3

start on CONTRIBUTING.md

cf4795b

rename metadata sub-commands

8e8cef8

make canonize more flexible

b9b5e6f

gmlewis approved these changes Oct 16, 2023

View reviewed changes

WillAbides added 2 commits October 19, 2023 13:26

use //meta:operation directives

b2b2e71

Merge branch 'master' into meta4

54443b7

# Conflicts: # update-urls/go.mod # update-urls/go.sum

WillAbides marked this pull request as ready for review October 19, 2023 18:41

WillAbides requested a review from gmlewis October 19, 2023 18:42

revert accidental test changes

ffd73b6

gmlewis requested changes Oct 19, 2023

View reviewed changes

gmlewis added the NeedsReview PR is awaiting a review before merging. label Oct 19, 2023

WillAbides and others added 4 commits October 19, 2023 14:29

Apply suggestions from code review

a2bff7b

Co-authored-by: Glenn Lewis <[email protected]>

Apply suggestions from code review

4183cf5

Co-authored-by: Glenn Lewis <[email protected]>

return AtoI errors

b6d3af6

update CONTRIBUTING.md

6e4d263

WillAbides requested a review from gmlewis October 19, 2023 19:52

gmlewis approved these changes Oct 19, 2023

View reviewed changes

WillAbides changed the title ~~Use metadata.yaml to reconcile go-github with GitHub's OpenAPI descriptions~~ Use metadata to reconcile go-github with GitHub's OpenAPI descriptions Oct 23, 2023

WillAbides added 2 commits October 23, 2023 11:13

Merge branch 'master' into meta4

0204952

# Conflicts: # github/github.go

add meta for RateLimitService.Get

66a2035

vandanrohatgi approved these changes Oct 31, 2023

View reviewed changes

valbeat approved these changes Nov 1, 2023

View reviewed changes

WillAbides added 2 commits November 3, 2023 09:17

Merge branch 'master' into meta4

d72f064

# Conflicts: # github/rate_limit.go

fix .golangci.yml exclusion for tools

4a065a9

gmlewis merged commit a54bc7d into google:master Nov 3, 2023
7 checks passed

gmlewis removed the NeedsReview PR is awaiting a review before merging. label Dec 1, 2023

gmlewis mentioned this pull request Dec 1, 2023

Bump version of go-github to v57.0.0 #3009

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use metadata to reconcile go-github with GitHub's OpenAPI descriptions #2919

Use metadata to reconcile go-github with GitHub's OpenAPI descriptions #2919

WillAbides commented Sep 9, 2023 •

edited

Loading

codecov bot commented Sep 9, 2023 •

edited

Loading

gmlewis left a comment

WillAbides commented Sep 9, 2023

WillAbides commented Sep 10, 2023

gmlewis left a comment

WillAbides commented Oct 16, 2023

gmlewis commented Oct 16, 2023 •

edited

Loading

WillAbides commented Oct 16, 2023

gmlewis left a comment

gmlewis Oct 19, 2023

gmlewis Oct 19, 2023

WillAbides Oct 19, 2023

gmlewis left a comment

vandanrohatgi left a comment

gmlewis commented Nov 3, 2023

		major, _ := strconv.Atoi(m[pattern.SubexpIndex("major")])
		minor, _ := strconv.Atoi(m[pattern.SubexpIndex("minor")])

Use metadata to reconcile go-github with GitHub's OpenAPI descriptions #2919

Use metadata to reconcile go-github with GitHub's OpenAPI descriptions #2919

Conversation

WillAbides commented Sep 9, 2023 • edited Loading

codecov bot commented Sep 9, 2023 • edited Loading

Codecov Report

gmlewis left a comment

Choose a reason for hiding this comment

WillAbides commented Sep 9, 2023

WillAbides commented Sep 10, 2023

gmlewis left a comment

Choose a reason for hiding this comment

WillAbides commented Oct 16, 2023

gmlewis commented Oct 16, 2023 • edited Loading

WillAbides commented Oct 16, 2023

gmlewis left a comment

Choose a reason for hiding this comment

gmlewis Oct 19, 2023

Choose a reason for hiding this comment

gmlewis Oct 19, 2023

Choose a reason for hiding this comment

WillAbides Oct 19, 2023

Choose a reason for hiding this comment

gmlewis left a comment

Choose a reason for hiding this comment

vandanrohatgi left a comment

Choose a reason for hiding this comment

gmlewis commented Nov 3, 2023

WillAbides commented Sep 9, 2023 •

edited

Loading

codecov bot commented Sep 9, 2023 •

edited

Loading

gmlewis commented Oct 16, 2023 •

edited

Loading