-
Notifications
You must be signed in to change notification settings - Fork 899
maint: Updates go-github dep to v77 #2891
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
base: main
Are you sure you want to change the base?
Conversation
…reduce change confusion
- Implements CRUD operations for GitHub organization network configurations
- Supports configuration of network settings for hosted compute (Actions)
- Validates name format (alphanumeric, '.', '-', '_') and length (1-100 chars)
- Enforces compute_service enum ('none', 'actions')
- Requires exactly one network_settings_id
- Includes comprehensive test suite with create, update, and import tests
- Depends on go-github v77 (merged from PR integrations#2891)
Resolves network configuration management for GitHub Actions hosted runners
and other hosted compute services at the organization level.
|
@nickfloyd I've noticed that you've swapped out team ids for slugs to deal with the deprecated methods but that will break the provider for anyone using it at any serious scale. The fix here is to deprecate the See #2497. |
4a9558d to
3ac34e3
Compare
|
@nickfloyd now that the provider is under maintenance I wouldn't be against adding the |
|
@nickfloyd is this ready for a review? |
Yes it is ❤️ . I'll need to sync the branch with #2902 once/if you approve my changes there. |
|
@nickfloyd I'll review this shortly. |
deiga
left a comment
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.
First light-weight pass, the project migration docs seem to be missing?
This diff makes me wonder if we should mark the files in vendor/ as generated files with .gitattributes to make these diffs slightly easier. Not part of this PR of course
| if len(expandedRules.FilePathRestriction.RestrictedFilePaths) != 3 { | ||
| t.Errorf("Expected 3 restricted file paths, got %d", len(expandedRules.FilePathRestriction.RestrictedFilePaths)) | ||
| } |
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.
question: Would it make sense to use something like len(rulesMap["file_path_restriction"][0]["file_path_restriction"]) instead of hard-coding the number 3 here?
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.
This is really following the current idioms in the provider. It appears that the pattern might've set up for readability and determinism, as well as simplicity of implementation. We can certainly look at changing these down the road to establish a pattern.
| if expandedRules.MaxFileSize.MaxFileSize != 100 { | ||
| t.Errorf("Expected max file size to be 100, got %d", expandedRules.MaxFileSize.MaxFileSize) | ||
| } | ||
| } | ||
| if expandedRules.MaxFilePathLength != nil { | ||
| ruleCount++ | ||
| if expandedRules.MaxFilePathLength.MaxFilePathLength != 250 { | ||
| t.Errorf("Expected max file path length to be 250, got %d", expandedRules.MaxFilePathLength.MaxFilePathLength) | ||
| } | ||
| } | ||
| if expandedRules.FileExtensionRestriction != nil { | ||
| ruleCount++ | ||
| if len(expandedRules.FileExtensionRestriction.RestrictedFileExtensions) != 4 { |
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.
question: Same for these conditionals. Would it make sense to refer the rulesMap values directly instead of hard-coding them?
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.
My opinion above is why I would not favor changing these right now but if we deem that as important then I'd be up for considering it.
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.
@nickfloyd it's worth noting that the duplication of the expected value in the test and the error message caused quite a lot of overhead in the patch alignment PR. I'm also not sure I agree that this can be classed as an idiom and would be tempted to follow the boy scout rule and fix it as part of the changes.
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.
Absolutely. I can totally get behind that. My concern is that this PR is already huge - so I can scope the modifications to just this file and any others related to it - we should follow up with a cleaner approach across the entire provider.
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.
Side note. Since we moved away from interface{} the logic to prop dig is kinda gnarly - perhaps there is a better way to do this:
filePathRule := rulesMap["file_path_restriction"].([]any)[0].(map[string]any)
expectedFilePaths := len(filePathRule["restricted_file_paths"].([]any))
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.
Side note. Since we moved away from interface{} the logic to prop dig is kinda gnarly - perhaps there is a better way to do this:
Do you mean the use of any over interface{}?
| @@ -9,6 +9,8 @@ description: |- | |||
|
|
|||
| !> **Warning:** This resource no longer works as the [Projects (classic) REST API](https://docs.github.com/en/rest/projects/projects?apiVersion=2022-11-28) has been [removed](https://github.blog/changelog/2024-05-23-sunset-notice-projects-classic/) and as such has been deprecated. It will be removed in a future release. | |||
|
|
|||
| ~> **Migration:** Please use the [`github_project_item`](./project_item.html) resource for GitHub Projects V2 instead. | |||
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.
issue: This file seems to be missing?
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 think this doc needs removing as the projects V2 resources are in a separate PR and there is no migration path.
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.
Yeah this was a left over artifact from my conversion and then later sundering it into it's own PR. I'll remove it. Great catch!
| var ruleset *github.Ruleset | ||
| var ruleset *github.RepositoryRuleset | ||
| var err error | ||
|
|
||
| ruleset, _, err = client.Organizations.CreateOrganizationRuleset(ctx, owner, rulesetReq) | ||
| ruleset, _, err = client.Organizations.CreateRepositoryRuleset(ctx, org, *rulesetReq) |
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.
note: Boy, this choice of naming was poor from the SDK. I had to read the API and SDK contents to make sure it's still doing the right thing 🙃
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.
Agreed. I got this one wrong about 3 times because of it. Other than making the changes in the SDK itself I think we want to be cautious with our APIs as a result - balancing pragmatism with DX.
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.
Naming is hard so GitHub likes to reuse names with slight contextual modifiers. The new function name is clearer in the context of the GitHub API as the old name duplicated the service context (organization) into the function name and didn't distinguish between ruleset types (which in fairness wasn't a problem when it was originally named). FYI I work on the SDK.
| // Use UpdateRulesetNoBypassActor here instead of UpdateRuleset *if* bypass_actors has changed. | ||
| // UpdateRuleset uses `omitempty` on BypassActors, causing empty arrays to be omitted from the JSON. | ||
| // UpdateRulesetNoBypassActor always includes the field so that bypass actors can actually be removed. | ||
| // See: https://github.com/google/go-github/blob/b6248e6f6aec019e75ba2c8e189bfe89f36b7d01/github/repos_rules.go#L196 |
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.
question: This comment doesn't seem to be correct anymore, since we don't use UpdateRulesetNoBypassActor now. Is it otherwise still relevant?
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.
Great catch / point. This was a changeset merged in after the migration. I'll clean it up ❤️
|
|
||
| allTeams := make([]any, 0) | ||
|
|
||
| //nolint:staticcheck // SA1019: ListSecurityManagerTeams is deprecated but still needed for legacy compatibility |
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.
question: Do we have a plan when to migrate this to use ListTeamsAssignedToOrgRole instead? It seems Jan 1st ListSecurityManagerTeams might stop working?
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. Well sort of, there is an issue. @stevehipwell rightfully called out the issues with migrating to the team_slug from team_ids. RE: #2497
stevehipwell
left a comment
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've added some review comments.
| client := meta.(*Owner).v3client | ||
| orgID := meta.(*Owner).id | ||
| owner := meta.(*Owner).name | ||
| orgId := meta.(*Owner).id |
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.
| orgId := meta.(*Owner).id | |
| orgID := meta.(*Owner).id |
Why did you change this? In Go the idiomatic capitalization is <thing>ID.
| } | ||
|
|
||
| errs := []string{fmt.Sprintf("could not update the Pull Request: %v", err)} | ||
| errors := []string{fmt.Sprintf("could not update the Pull Request: %v", err)} |
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.
| errors := []string{fmt.Sprintf("could not update the Pull Request: %v", err)} | |
| errs := []string{fmt.Sprintf("could not update the Pull Request: %v", err)} |
Why did you change this? This would shadow the errors package in the stdlib and even if it's not currently being used in this file it's not idiomatic.
|
|
||
| client := meta.(*Owner).v3client | ||
| orgId := meta.(*Owner).id | ||
| teamSlug := d.Get("slug").(string) |
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'm not sure where slug is defined? I suspect this may be left over from some previous changes?
| teamId, err := getTeamID(teamIdString, meta) | ||
| if err != nil { | ||
| return err | ||
| } |
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 think this might need putting back?
| } | ||
|
|
||
| repo, resp, repoErr := client.Teams.IsTeamRepoByID(ctx, orgId, teamId, orgName, repoName) | ||
| repo, resp, repoErr := client.Teams.IsTeamRepoBySlug(ctx, orgName, teamSlug, orgName, repoName) |
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 think this might need undoing?
| ruleset, resp, err = client.Organizations.GetOrganizationRuleset(ctx, owner, rulesetID) | ||
| ruleset, resp, err = client.Organizations.GetRepositoryRuleset(ctx, org, rulesetID) | ||
| if err != nil { | ||
| ghErr := &github.ErrorResponse{} |
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.
This code pattern looks to be fundamentally flawed as we've got an error and are not handling it, I suspect we'd find it throughout the codebase. In this case we don't return the error and the ruleset is empty so we remove it from state which can result in issues like #2889.
| return nil | ||
| } | ||
|
|
||
| if ruleset == nil { |
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.
This duplication looks like a typo?
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.
Oof good catch.
| for _, t := range want { | ||
| teamData := t.(map[string]any) | ||
| teamIDString := teamData["team_id"].(string) | ||
| teamSlug, err := getTeamSlug(teamIDString, meta) |
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 think this might be left over from some removed changes?
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.
Yeah, great catch... it was missed when I was making changes after your comment.
| permission := teamData["permission"].(string) | ||
| // team needs to be added | ||
| log.Printf("[DEBUG] Adding team %s with permission %s for repo: %s.", teamIDString, permission, repoName) | ||
| log.Printf("[DEBUG] Adding team %s with permission %s for repo: %s.", teamSlug, permission, repoName) |
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.
We shouldn't have the team slug due to the overhead of doing so, so this should be using the team id.
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.
Looks like I missed one during the rollback.. thx for the heads up.
| }, | ||
|
|
||
| SchemaVersion: 1, | ||
| SchemaVersion: 2, |
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.
As org ruleset comment.
Resolves: #2809, #2602
Relates to: #2898
This change set is large. This was an intentional decision to drive forward our need to update go-github given the current provider was 10 or more versions behind the latest. The hope is that the release will encapsulate all of the braking changes over the last 12 months. This helps to make us just deal with them once if we choose to move forward on the major version bump. The last two releases were packed with much needed changes - so users, if they choose, can migrate as time allows while using the updates from v6.7.x and v6.8.x.
Additionally, we'll most likely cherry pick out feature sets from this branch that are not actual fixes and the like - i.e. projects APIs have stopped working so we can look at projects v2 as a new feature.
Before the change?
After the change?
Checklist
[x] Convert/Replace projects to projects v2(feat: Projects v2 project items & go-github v79 #2898)Pull request checklist
Does this introduce a breaking change?
Please see our docs on breaking changes to help!