feat: Add manage_ghes endpoints introduced in 3.15 #3433

s4mur4i · 2025-01-15T18:02:32Z

Fixes: #3415.

codecov · 2025-01-15T19:19:28Z

Codecov Report

Attention: Patch coverage is 94.00000% with 15 lines in your changes missing coverage. Please review.

Project coverage is 92.29%. Comparing base (4510383) to head (b07fc10).
Report is 5 commits behind head on master.

Files with missing lines	Patch %	Lines
github/enterprise_manage_ghes.go	88.88%	4 Missing and 2 partials ⚠️
github/enterprise_manage_ghes_config.go	95.16%	4 Missing and 2 partials ⚠️
github/enterprise_manage_ghes_maintenance.go	90.00%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3433      +/-   ##
==========================================
+ Coverage   92.26%   92.29%   +0.02%     
==========================================
  Files         174      178       +4     
  Lines       15023    15273     +250     
==========================================
+ Hits        13861    14096     +235     
- Misses       1068     1078      +10     
- Partials       94       99       +5

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

gmlewis · 2025-01-15T20:14:59Z

Please run ./script/generate.sh and ./script/fmt.sh and push the changes to this PR. Thanks.

s4mur4i · 2025-01-15T21:23:06Z

Please run ./script/generate.sh and ./script/fmt.sh and push the changes to this PR. Thanks.

Strangely it doesn't show any diff for me:

➜  go-github git:(master) ./script/generate.sh                                                                
➜  go-github git:(master) ./script/fmt.sh                                                                     
➜  go-github git:(master) git status                                                                         
On branch master
Your branch is up to date with 'origin/master'.

nothing to commit, working tree clean

Even lint shows no diff:

➜  go-github git:(master) ./script/lint.sh                                                                   
linting .
linting example
linting scrape
linting tools
validating generated files

gmlewis · 2025-01-15T21:28:18Z

Can you see this? https://github.com/google/go-github/actions/runs/12794271004/job/35672748807?pr=3433

gmlewis

Thank you, @s4mur4i .

This is a very large PR, and it looks like there are going to be many things to address, so I'll stop my review here and hopefully we will get the linting issues worked out.

Please see CONTRIBUTING.md for more helpful information.

github/enterprise_manage_ghes.go

s4mur4i · 2025-01-15T21:52:01Z

Can you see this? https://github.com/google/go-github/actions/runs/12794271004/job/35672748807?pr=3433

Yes, I can. I don't really understand why I don't see change.

gmlewis · 2025-01-15T21:57:01Z

Can you see this? https://github.com/google/go-github/actions/runs/12794271004/job/35672748807?pr=3433

Yes, I can. I don't really understand why I don't see change.

One thing that I don't typically see in PRs is that you didn't create a separate branch for this change, but instead stayed on "master". I'm not sure if that is a problem or not, but it is one thing I don't think I've ever seen before. Typically, after you fork a repo and want to create a PR, you create a new branch and perform your work on that branch, then make a pull request off of that branch. Maybe this is nothing... I don't know... it's just different.

s4mur4i · 2025-01-15T22:04:21Z

Can you see this? https://github.com/google/go-github/actions/runs/12794271004/job/35672748807?pr=3433

Yes, I can. I don't really understand why I don't see change.

One thing that I don't typically see in PRs is that you didn't create a separate branch for this change, but instead stayed on "master". I'm not sure if that is a problem or not, but it is one thing I don't think I've ever seen before. Typically, after you fork a repo and want to create a PR, you create a new branch and perform your work on that branch, then make a pull request off of that branch. Maybe this is nothing... I don't know... it's just different.

I cloned in a new directory and worked. strange. but should be good now.

s4mur4i · 2025-01-16T14:46:34Z

@gmlewis Regarding generate. I dont know how to solve that issue.
I tried with one of my colleagues, and he has same output and behaviour.
I don't know if it might be because of mac-os.

gmlewis · 2025-01-16T14:55:44Z

@gmlewis Regarding generate. I dont know how to solve that issue. I tried with one of my colleagues, and he has same output and behaviour. I don't know if it might be because of mac-os.

Hmmm... very strange. I'm on a Mac too and have not seen this before.

s4mur4i · 2025-01-16T15:00:20Z

I reinstalled go o match exact version:

go-github git:(master) ✗ go version                                                                                                                                                           
go version go1.22.10 darwin/arm64

Pushed the update.

gmlewis · 2025-01-16T15:58:02Z

@stevehipwell - @alexandear - @dnwe - @willnorris - with our recently changes to Go Toolchain settings, we are now seeing bizarre problems like this one where the tooling did not help out the developer and made for a frustrating formatting experience. GitHub Workflows linting conflicted with the developer linting.

We need to come up with a solution that doesn't hurt the developer experience and yet is easy to maintain moving forward.

seruman · 2025-01-16T16:31:00Z

.. I tried with one of my colleagues, and he has same output and behaviour.

Colleague here! I've took a look at this and here're my findings;

SMTP struct has two fields; UserName and Username, looks like Github API returns both username and user_name 🤷

https://docs.github.com/[email protected]/rest/enterprise-admin/manage-ghes#get-the-ghes-settings

https://github.com/s4mur4i/go-github/blob/a6383545144dd8f98845c94f4c453c920cb82046/github/enterprise_manage_ghes_config.go#L241-L242

The issue seems to be, for both fields getter.sortVal generated to has same value, lower case receiver + field;

go-github/github/gen-accessors.go

Line 221 in a638354

sortVal: strings.ToLower(receiverType) + "." + strings.ToLower(fieldName),

Since sort.Sort is not stable, it might sort these two getter's different on each run;

go-github/github/gen-accessors.go

Lines 184 to 185 in a638354

    
           // Sort getters by ReceiverType.FieldName. 
        
           sort.Sort(byName(t.Getters))

dnwe · 2025-01-16T16:33:35Z

@gmlewis I think #3436 is likely the required fix here

gmlewis · 2025-01-16T16:56:44Z

@s4mur4i - can you please manually make the changes from #3436 to this PR and see if that solves the problem for you?

s4mur4i · 2025-01-16T18:29:14Z

I accidently did a merge commit, just wanted to fix a resolve conflict :(
I applied the fixes from Pr #3436

gmlewis · 2025-01-16T19:26:38Z

I accidently did a merge commit, just wanted to fix a resolve conflict :( I applied the fixes from Pr #3436

No worries about the merge commit in this repo. We always squash-and-merge and therefore always have a clean commit history. This is also why we ask people to please not use force-push in this repo, as it makes it much more difficult for reviewers to see what has changed since their last review.

s4mur4i · 2025-01-16T21:26:16Z

As I see there is still lint error. I guess then the toolchain was not a fix for this case.

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

gmlewis · 2025-01-17T16:49:59Z

One question for myself. Contribution.md says not to use merge commit, but would say to do cherry-pick or rebase. I did a rebase to get the patch and to get latest state. but a rebase would require a force-push, I don't know of other way to do it.
Should I have just taken patch applied it manually, or is there any other method recommended?

While you are working on a PR, you don't need to worry about what the master branch is doing.
Just keep pushing commits to your PR and please do NOT force-push.

We ALWAYS use "Squash and merge" in this repo, so our commit history is always clean. Do a "git log" on this repo to see what I mean.

If you ever have conflicting changes and GitHub says that the PR needs updating, you can simply follow these steps:

git checkout master
git pull origin master
git checkout my-custom-cool-branch-that-I-am-currently-working-on
git merge master

If there are conflicts, I personally like to use git mergetool (with kdiff3 as my merge tool) and resolve all the conflicts and then finalize the resolution with the command git commit -sam 'git merge master'.

git push origin my-custom-cool-branch-that-I-am-currently-working-on

This may add many commits to a PR, but in the end, it never matters because we always "Squash and merge".

Please let me know if any of this is not clear.

I tried to explain this clearly in CONTRIBUTING.md, but if it is still unclear, please send me a PR to clean up the wording so that it is super-easy to understand.
Thank you.

s4mur4i · 2025-01-17T19:38:41Z

@gmlewis I have an interesting issue, which I don't fully understand why.
I am working on 2 Comments. I have following Patch:

git diff github/enterprise_manage_ghes.go                                                                                                                                           cost-cluster
diff --git a/github/enterprise_manage_ghes.go b/github/enterprise_manage_ghes.go
index 3ecf373f..f4e136d8 100644
--- a/github/enterprise_manage_ghes.go
+++ b/github/enterprise_manage_ghes.go
@@ -27,13 +27,13 @@ type ClusterStatus struct {

 // ClusterStatusNode represents the status of a cluster node.
 type ClusterStatusNode struct {
-	Hostname *string                       `json:"hostname,omitempty"`
-	Status   *string                       `json:"status,omitempty"`
-	Services []*ClusterStatusNodesServices `json:"services"`
+	Hostname *string                      `json:"hostname,omitempty"`
+	Status   *string                      `json:"status,omitempty"`
+	Services []*ClusterStatusNodesService `json:"services"`
 }

-// ClusterStatusNodesServices represents the status of a service running on a cluster node.
-type ClusterStatusNodesServices struct {
+// ClusterStatusNodesService represents the status of a service running on a cluster node.
+type ClusterStatusNodesService struct {
 	Status  *string `json:"status,omitempty"`
 	Name    *string `json:"name,omitempty"`
 	Details *string `json:"details,omitempty"`

When I run generate I get following error:

./script/generate.sh                                                                                                                                                                cost-cluster
no operations defined for ClusterStatusNodesService.GetDetails
no operations defined for ClusterStatusNodesService.GetName
no operations defined for ClusterStatusNodesService.GetStatus
github/github.go:8: running "../script/metadata.sh": exit status 1

Same thing happens when I edit ConnectionService
Do you have idea why it might happen? All other generates were successful, just these 2 are having issues.
In github-accessors.go I see files successfully generated:

git diff github/github-accessors.go                                                                                                                                                 cost-cluster
diff --git a/github/github-accessors.go b/github/github-accessors.go
index d7841914..a020bcb4 100644
--- a/github/github-accessors.go
+++ b/github/github-accessors.go
@@ -2527,7 +2527,7 @@ func (c *ClusterStatusNode) GetStatus() string {
 }

 // GetDetails returns the Details field if it's non-nil, zero value otherwise.
-func (c *ClusterStatusNodesServices) GetDetails() string {
+func (c *ClusterStatusNodesService) GetDetails() string {
 	if c == nil || c.Details == nil {
 		return ""
 	}
@@ -2535,7 +2535,7 @@ func (c *ClusterStatusNodesServices) GetDetails() string {
 }

 // GetName returns the Name field if it's non-nil, zero value otherwise.
-func (c *ClusterStatusNodesServices) GetName() string {
+func (c *ClusterStatusNodesService) GetName() string {
 	if c == nil || c.Name == nil {
 		return ""
 	}
@@ -2543,7 +2543,7 @@ func (c *ClusterStatusNodesServices) GetName() string {
 }

 // GetStatus returns the Status field if it's non-nil, zero value otherwise.
-func (c *ClusterStatusNodesServices) GetStatus() string {
+func (c *ClusterStatusNodesService) GetStatus() string {
 	if c == nil || c.Status == nil {
 		return ""
 	}

Even stranger if I change it to ClusterStatusNodesServicess it works, and also ClusterStatusNodesServic works.

gmlewis · 2025-01-17T19:54:10Z

#3437 was just merged into master - can you please try it out to see if that fixes the problems you are seeing?

s4mur4i · 2025-01-17T20:07:54Z

#3437 was just merged into master - can you please try it out to see if that fixes the problems you are seeing?

This morning this got merged into my branch:
s4mur4i@d13c739
it is not working with it.

gmlewis · 2025-01-17T20:10:32Z

#3437 was just merged into master - can you please try it out to see if that fixes the problems you are seeing?

This morning this got merged into my branch: s4mur4i@d13c739 it is not working with it.

@alexandear - can you please comment?

alexandear · 2025-01-20T11:27:18Z

@gmlewis I have an interesting issue, which I don't fully understand why. I am working on 2 Comments. I have following Patch:
...

When I run generate I get following error:
./script/generate.sh                                                                                                                                                                cost-cluster
no operations defined for ClusterStatusNodesService.GetDetails
no operations defined for ClusterStatusNodesService.GetName
no operations defined for ClusterStatusNodesService.GetStatus
github/github.go:8: running "../script/metadata.sh": exit status 1
Same thing happens when I edit ConnectionService

Even stranger if I change it to ClusterStatusNodesServicess it works, and also ClusterStatusNodesServic works.

The issue appears because the struct suffixed with Service. It's because generate.sh executes metadata.sh, see

go-github/github/github.go

Line 8 in d13c739

//go:generate ../script/metadata.sh update-go

Every exported type where type name ends in Service, see

go-github/tools/metadata/metadata.go

Line 515 in d13c739

    
           if !id.IsExported() || !fn.Name.IsExported() || !strings.HasSuffix(id.Name, "Service") {

Service type is a special type in go-github for grouping related API functions. Examples: IssuesService, EnterpriseService etc.

To workaround this, we can rename ClusterStatusNodesServices to ClusterStatusNodesServiceItem, ConnectionServices to ConnectionServiceItem.

s4mur4i · 2025-01-20T13:40:58Z

@alexandear Thank you for the update. With your solution it worked.
@gmlewis I think all comments were fixed, please let me know if something is missing.
Should I resolve the comments or will you do after reviewing?

gmlewis · 2025-01-20T13:42:32Z

Should I resolve the comments or will you do after reviewing?

Feel free to resolve them or I can later. Either way is fine with me. 😁

gmlewis · 2025-01-20T13:45:27Z

@s4mur4i - I'm currently on my cell phone but should be able to review this later today.
In the meantime, would you mind reviewing other PRs in this repo that have the "Needs Review" label if you have time? Thank you!

s4mur4i · 2025-01-20T14:43:30Z

I resolved the one where I added a fix.
Also I checked to see Needs Review tag.
One of them was last year October, you wrote, if they don't respond you will close the ticket. didn't see response or action there.
Another one, same October, also no response
And last one just got an update from Alexander.
I think all are handled, and currently no action?

gmlewis · 2025-01-20T15:21:04Z

I resolved the one where I added a fix. Also I checked to see Needs Review tag. One of them was last year October, you wrote, if they don't respond you will close the ticket. didn't see response or action there. Another one, same October, also no response And last one just got an update from Alexander. I think all are handled, and currently no action?

Please feel free to ping me on PRs or issues where you see inconsistencies like this. Thank you, @s4mur4i !

gmlewis · 2025-01-20T16:38:48Z

Hey @stevehipwell - I haven't seen this in ages!!! Thank you for fixing this!!!

gmlewis

Half-way through the second file. Let's stop here so you can address another sweeping change I've requested to one-word struct names.

github/enterprise_manage_ghes.go

github/enterprise_manage_ghes_config.go

github/enterprise_manage_ghes_maintenance.go

github/enterprise_manage_ghes.go

github/enterprise_manage_ghes_config.go

github/enterprise_manage_ghes.go

github/enterprise_manage_ghes_maintenance.go

github/enterprise_manage_ghes_ssh.go

s4mur4i · 2025-01-21T14:53:25Z

@gmlewis The breaking test I think is not related to my work?

gmlewis

Thank you, @s4mur4i!
LGTM.

Next time, we should ideally break up enormous PRs like this one into several smaller PRs.

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

stevehipwell

LGTM

gmlewis · 2025-01-21T17:19:33Z

Thank you, @stevehipwell !
Merging.

s4mur4i · 2025-01-21T18:20:20Z

Thank you for the reviews, and help!

gmlewis changed the title ~~feat: Adding manage_ghes endpoints introduced in 3.15~~ feat: Add manage_ghes endpoints introduced in 3.15 Jan 15, 2025

gmlewis requested changes Jan 15, 2025

View reviewed changes

gmlewis added the NeedsReview PR is awaiting a review before merging. label Jan 15, 2025

alexandear mentioned this pull request Jan 16, 2025

gen-accessors: update dumping of getters #3437

Merged

s4mur4i and others added 9 commits January 17, 2025 10:20

Adding manage_ghes endpoints introduced in 3.15

b0d92eb

Fix year in Copyright

b0f90be

Fix generate & lint

e75c400

Fix double-space in comment

9a0776c

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

Fix omitempty and types

c024efa

Fix ghes_ssh Modified field

65cea53

Fix broken test

519df01

Add missing testNewRequestAndDoFailure

cdb712e

Run generate

9775d73

s4mur4i added 2 commits January 17, 2025 22:18

Fix ClusterSSHKey

f3d10fd

Fix enterprise_manage_ghes_config

c4121e9

Fix ClusterStatusNodeServiceItem and ConnectionServiceItem

b38b74f

Merge branch 'master' into master

a469388

gmlewis requested changes Jan 20, 2025

View reviewed changes

Fix for review findings in config

957be90

gmlewis reviewed Jan 20, 2025

View reviewed changes

github/enterprise_manage_ghes_config.go Outdated Show resolved Hide resolved

Revert int64 changes for non ID fields

9be9a4c

gmlewis requested changes Jan 21, 2025

View reviewed changes

alexandear suggested changes Jan 21, 2025

View reviewed changes

Fix for review findings

b07fc10

gmlewis approved these changes Jan 21, 2025

View reviewed changes

stevehipwell approved these changes Jan 21, 2025

View reviewed changes

gmlewis removed the NeedsReview PR is awaiting a review before merging. label Jan 21, 2025

gmlewis merged commit daaa51a into google:master Jan 21, 2025
7 checks passed

feat: Add manage_ghes endpoints introduced in 3.15 #3433

feat: Add manage_ghes endpoints introduced in 3.15 #3433

Conversation

s4mur4i commented Jan 15, 2025 • edited by gmlewis Loading

codecov bot commented Jan 15, 2025 • edited Loading

Codecov Report

gmlewis commented Jan 15, 2025

s4mur4i commented Jan 15, 2025

gmlewis commented Jan 15, 2025

gmlewis left a comment

Choose a reason for hiding this comment

s4mur4i commented Jan 15, 2025

gmlewis commented Jan 15, 2025

s4mur4i commented Jan 15, 2025

s4mur4i commented Jan 16, 2025

gmlewis commented Jan 16, 2025

s4mur4i commented Jan 16, 2025

gmlewis commented Jan 16, 2025

seruman commented Jan 16, 2025 • edited Loading

dnwe commented Jan 16, 2025

gmlewis commented Jan 16, 2025

s4mur4i commented Jan 16, 2025

gmlewis commented Jan 16, 2025

s4mur4i commented Jan 16, 2025

gmlewis commented Jan 17, 2025 • edited Loading

s4mur4i commented Jan 17, 2025 • edited Loading

gmlewis commented Jan 17, 2025

s4mur4i commented Jan 17, 2025

gmlewis commented Jan 17, 2025

alexandear commented Jan 20, 2025 • edited Loading

s4mur4i commented Jan 20, 2025

gmlewis commented Jan 20, 2025

gmlewis commented Jan 20, 2025

s4mur4i commented Jan 20, 2025

gmlewis commented Jan 20, 2025

gmlewis commented Jan 20, 2025

gmlewis left a comment

Choose a reason for hiding this comment

s4mur4i commented Jan 21, 2025

gmlewis left a comment

Choose a reason for hiding this comment

stevehipwell left a comment

Choose a reason for hiding this comment

gmlewis commented Jan 21, 2025

s4mur4i commented Jan 21, 2025

s4mur4i commented Jan 15, 2025 •

edited by gmlewis

Loading

codecov bot commented Jan 15, 2025 •

edited

Loading

seruman commented Jan 16, 2025 •

edited

Loading

gmlewis commented Jan 17, 2025 •

edited

Loading

s4mur4i commented Jan 17, 2025 •

edited

Loading

alexandear commented Jan 20, 2025 •

edited

Loading