-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
feat: Add manage_ghes endpoints introduced in 3.15 #3433
base: master
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3433 +/- ##
==========================================
- Coverage 97.72% 92.29% -5.43%
==========================================
Files 153 178 +25
Lines 13390 15273 +1883
==========================================
+ Hits 13085 14096 +1011
- Misses 215 1078 +863
- Partials 90 99 +9 ☔ View full report in Codecov by Sentry. |
Please run |
Strangely it doesn't show any diff for me:
Even lint shows no diff:
|
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.
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.
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. |
@gmlewis Regarding generate. I dont know how to solve that issue. |
Hmmm... very strange. I'm on a Mac too and have not seen this before. |
I reinstalled go o match exact version:
Pushed the update. |
@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. |
Colleague here! I've took a look at this and here're my findings;
The issue seems to be, for both fields go-github/github/gen-accessors.go Line 221 in a638354
Since go-github/github/gen-accessors.go Lines 184 to 185 in a638354
|
I accidently did a merge commit, just wanted to fix a resolve conflict :( |
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. |
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 <6598971+gmlewis@users.noreply.github.com>
I ran generate and fmt, lint shows no change for me. hopefully it is good now. |
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 have to step out of office for a while, but this review is taking a lot of time, so I will go ahead and give feedback on the one file I've looked at so far.
Please remember to not use force-push in this repo moving forward in order to save me review time for this large PR. Thanks.
github/enterprise_manage_ghes.go
Outdated
// ClusterStatus represents a response from the GetClusterStatus and GetReplicationStatus method. | ||
type ClusterStatus struct { | ||
Status *string `json:"status,omitempty"` | ||
Nodes []*ClusterStatusNodes `json:"nodes"` |
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.
Nodes []*ClusterStatusNodes `json:"nodes"` | |
Nodes []*ClusterStatusNode `json:"nodes"` |
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.
The reason I decided to go with Plural is that its an array, and would normally hold more nodes. The reason for the change would be that the struc defines a single Node and its more convention?
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.
Fixed in d18f95d . PTAL
github/enterprise_manage_ghes.go
Outdated
// ClusterStatusNodes represents the status of a cluster node. | ||
type ClusterStatusNodes struct { |
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.
// ClusterStatusNodes represents the status of a cluster node. | |
type ClusterStatusNodes struct { | |
// ClusterStatusNode represents the status of a cluster node. | |
type ClusterStatusNode struct { |
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.
Fixed in d18f95d . PTAL
type ClusterStatusNodes struct { | ||
Hostname *string `json:"hostname,omitempty"` | ||
Status *string `json:"status,omitempty"` | ||
Services []*ClusterStatusNodesServices `json:"services"` |
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.
Services []*ClusterStatusNodesServices `json:"services"` | |
Services []*ClusterStatusNodesService `json:"services"` |
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.
Following logic from previous review, wouldnt ClusterStatusNodeService
to remove plural be better?
github/enterprise_manage_ghes.go
Outdated
return replicationStatus, resp, nil | ||
} | ||
|
||
// Versions gets the versions information deployed to each node. |
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.
// Versions gets the versions information deployed to each node. | |
// Versions gets the version information deployed to each node. |
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.
Fixed in d18f95d . PTAL
github/enterprise_manage_ghes.go
Outdated
// GitHub API docs: https://docs.github.com/enterprise-server@3.15/rest/enterprise-admin/manage-ghes#get-all-ghes-release-versions-for-all-nodes | ||
// | ||
//meta:operation GET /manage/v1/version | ||
func (s *EnterpriseService) Versions(ctx context.Context, opts *NodeQueryOptions) ([]*NodeReleaseVersions, *Response, error) { |
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.
func (s *EnterpriseService) Versions(ctx context.Context, opts *NodeQueryOptions) ([]*NodeReleaseVersions, *Response, error) { | |
func (s *EnterpriseService) GetNodeReleaseVersions(ctx context.Context, opts *NodeQueryOptions) ([]*NodeReleaseVersion, *Response, error) { |
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.
One question here. Since we are returning a []*NodeReleaseVersion
shouldn't name be GetNodeReleaseVersion
?
We are returning version from each node, so if output is singular, shouldn't the API call be also singular?
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.
No, because you get back a slice of zero or more versions returned.
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.
Okay, than you. Fixed in d18f95d . PTAL
github/enterprise_manage_ghes.go
Outdated
// NodeReleaseVersions represents a response from the GetReplicationStatus method. | ||
type NodeReleaseVersions struct { |
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.
// NodeReleaseVersions represents a response from the GetReplicationStatus method. | |
type NodeReleaseVersions struct { | |
// NodeReleaseVersion represents a response from the GetNodeReleaseVersions method. | |
type NodeReleaseVersion struct { |
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.
Fixed in d18f95d . PTAL
github/enterprise_manage_ghes.go
Outdated
Version *ReleaseVersions `json:"version"` | ||
} | ||
|
||
// ReleaseVersions holds the release version information of the node. | ||
type ReleaseVersions struct { |
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.
Version *ReleaseVersions `json:"version"` | |
} | |
// ReleaseVersions holds the release version information of the node. | |
type ReleaseVersions struct { | |
Version *ReleaseVersion `json:"version"` | |
} | |
// ReleaseVersion holds the release version information of the node. | |
type ReleaseVersion struct { |
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.
Fixed in d18f95d . PTAL
Version *string `json:"version,omitempty"` | ||
Platform *string `json:"platform,omitempty"` | ||
BuildID *string `json:"build_id,omitempty"` | ||
BuildDate *string `json:"build_date,omitempty"` |
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.
Should this be *Timestamp
instead of *string
? I'll need to check the official docs, but don't have time right now.
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.
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.
Yes, thats why I decided to use string. Dont know if Timestamp could have done a short date format.
type ConfigApplyStatus struct { | ||
Running *bool `json:"running,omitempty"` | ||
Successful *bool `json:"successful,omitempty"` | ||
Nodes []*ConfigApplyStatusNodes `json:"nodes"` |
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.
Nodes []*ConfigApplyStatusNodes `json:"nodes"` | |
Nodes []*ConfigApplyStatusNode `json:"nodes"` |
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.
Fixed in c4121e9. PTAL
// ConfigApplyStatusNodes is a struct to hold the response from the ConfigApply API. | ||
type ConfigApplyStatusNodes struct { |
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.
// ConfigApplyStatusNodes is a struct to hold the response from the ConfigApply API. | |
type ConfigApplyStatusNodes struct { | |
// ConfigApplyStatusNode is a struct to hold the response from the ConfigApply API. | |
type ConfigApplyStatusNode struct { |
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.
Fixed in c4121e9. PTAL
|
||
// ConfigApplyEvents is a struct to hold the response from the ConfigApplyEvents API. | ||
type ConfigApplyEvents struct { | ||
Nodes []*ConfigApplyEventsNodes `json:"nodes"` |
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.
Nodes []*ConfigApplyEventsNodes `json:"nodes"` | |
Nodes []*ConfigApplyEventsNode `json:"nodes"` |
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.
Fixed in c4121e9. PTAL
// ConfigApplyEventsNodes is a struct to hold the response from the ConfigApplyEvents API. | ||
type ConfigApplyEventsNodes struct { |
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.
// ConfigApplyEventsNodes is a struct to hold the response from the ConfigApplyEvents API. | |
type ConfigApplyEventsNodes struct { | |
// ConfigApplyEventsNode is a struct to hold the response from the ConfigApplyEvents API. | |
type ConfigApplyEventsNode struct { |
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.
Fixed in c4121e9. PTAL
type ConfigApplyEventsNodes struct { | ||
Node *string `json:"node,omitempty"` | ||
LastRequestID *string `json:"last_request_id,omitempty"` | ||
Events []*ConfigApplyEventsNodeEvents `json:"events"` |
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.
Events []*ConfigApplyEventsNodeEvents `json:"events"` | |
Events []*ConfigApplyEventsNodeEvent `json:"events"` |
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.
Fixed in c4121e9. PTAL
// ConfigApplyEventsNodeEvents is a struct to hold the response from the ConfigApplyEvents API. | ||
type ConfigApplyEventsNodeEvents struct { |
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.
// ConfigApplyEventsNodeEvents is a struct to hold the response from the ConfigApplyEvents API. | |
type ConfigApplyEventsNodeEvents struct { | |
// ConfigApplyEventsNodeEvent is a struct to hold the response from the ConfigApplyEvents API. | |
type ConfigApplyEventsNodeEvent struct { |
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.
Fixed in c4121e9. PTAL
UUID *string `json:"uuid,omitempty"` | ||
Status *string `json:"status,omitempty"` | ||
ScheduledTime *Timestamp `json:"scheduled_time,omitempty"` | ||
ConnectionServices []*ConnectionServices `json:"connection_services"` |
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.
ConnectionServices []*ConnectionServices `json:"connection_services"` | |
ConnectionServices []*ConnectionService `json:"connection_services"` |
// ConnectionServices represents the connection services for the maintenance status. | ||
type ConnectionServices struct { |
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.
// ConnectionServices represents the connection services for the maintenance status. | |
type ConnectionServices struct { | |
// ConnectionService represents the connection services for the maintenance status. | |
type ConnectionService struct { |
github/enterprise_manage_ghes_ssh.go
Outdated
// ClusterSSHKeys represents the SSH keys configured for the instance. | ||
type ClusterSSHKeys struct { |
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.
// ClusterSSHKeys represents the SSH keys configured for the instance. | |
type ClusterSSHKeys struct { | |
// ClusterSSHKey represents the SSH keys configured for the instance. | |
type ClusterSSHKey struct { |
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.
Fixed in f3d10fd . PTAL
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. |
Co-authored-by: Glenn Lewis <6598971+gmlewis@users.noreply.github.com>
While you are working on a PR, you don't need to worry about what the master branch is doing. 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:
If there are conflicts, I personally like to use
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. |
@gmlewis I have an interesting issue, which I don't fully understand why.
When I run generate I get following error:
Same thing happens when I edit
Even stranger if I change it to |
#3437 was just merged into |
This morning this got merged into my branch: |
@alexandear - can you please comment? |
The issue appears because the struct suffixed with Line 8 in d13c739
Every exported type where type name ends in Service , seego-github/tools/metadata/metadata.go Line 515 in d13c739
Service type is a special type in go-github for grouping related API functions. Examples: IssuesService , EnterpriseService etc.
To workaround this, we can rename |
closes #3415