Add ghprojects: declarative GitHub Projects V2 management#4815
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
931e49a to
9dae116
Compare
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- In
client.GetOrgID,q.Organization.IDis agithubv4.ID, soq.Organization.ID.(string)will panic/does not compile; it should be converted withstring(q.Organization.ID)to match how IDs are handled elsewhere. - In
updateProject, you pre-fetch fields and views viaGetProjectFields/GetProjectViewsand assign them toactual, butreconcileFields/reconcileViewsimmediately call those methods again; consider passing the already-fetched data into the reconcile functions to avoid redundant API calls.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `client.GetOrgID`, `q.Organization.ID` is a `githubv4.ID`, so `q.Organization.ID.(string)` will panic/does not compile; it should be converted with `string(q.Organization.ID)` to match how IDs are handled elsewhere.
- In `updateProject`, you pre-fetch fields and views via `GetProjectFields`/`GetProjectViews` and assign them to `actual`, but `reconcileFields`/`reconcileViews` immediately call those methods again; consider passing the already-fetched data into the reconcile functions to avoid redundant API calls.
## Individual Comments
### Comment 1
<location path="pkg/ghprojects/client/client.go" line_range="32" />
<code_context>
+ if err := c.gql.Query(ctx, &q, vars); err != nil {
+ return "", fmt.Errorf("querying org ID for %s: %w", login, err)
+ }
+ return q.Organization.ID.(string), nil
+}
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Avoid unsafe type assertion when converting githubv4.ID to string
`q.Organization.ID` is a `githubv4.ID`, not an interface, so `.(string)` will panic. Use `string(q.Organization.ID)` instead to convert safely and avoid a runtime panic.
</issue_to_address>
### Comment 2
<location path="pkg/ghprojects/reconcile/reconcile.go" line_range="13-17" />
<code_context>
+ "kubevirt.io/project-infra/pkg/ghprojects/config"
+)
+
+// Options controls reconciliation behavior.
+type Options struct {
+ Confirm bool // If false, dry-run only (log what would happen).
+ FixProjects bool // Create/update projects.
+ FixFields bool // Create/update/delete custom fields.
+ FixViews bool // Create/update views.
+}
</code_context>
<issue_to_address>
**issue:** Options comment overstates current field reconciliation capabilities
`FixFields` is documented as "Create/update/delete custom fields", but `reconcileFields` only creates missing fields and logs type mismatches; it doesn’t handle renames, option changes, or deletions. Please either adjust the comment to describe the current behavior or extend the implementation to match the option name/description so `--fix-fields` isn’t misleading.
</issue_to_address>
### Comment 3
<location path="pkg/ghprojects/dump/dump.go" line_range="64-65" />
<code_context>
+ })
+ }
+
+ key := slugify(p.Title)
+ orgProjects.Projects[key] = proj
+ }
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Slug generation can lead to empty or colliding project keys
Using `slugify(p.Title)` as the map key can yield an empty string (titles without alphanumerics) or collisions (different titles normalizing to the same slug), which will silently overwrite entries in `orgProjects.Projects`. Consider detecting empty/colliding slugs and either deriving a unique key (e.g., append project number) or returning an error so callers can handle it.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Scaffolds a new package and CLI tool for managing GitHub Projects V2 boards from YAML configuration, following the same desired-state reconciliation pattern as peribolos. Includes: - config: YAML types and loader with validation - client: GitHub GraphQL API wrapper for Projects V2 (projects, fields, views) - reconcile: desired-state reconciler with dry-run, per-resource fix flags - dump: export live GitHub project state back to YAML - robots/ghprojects: CLI entrypoint with sync and dump subcommands Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Lee Yarwood <lyarwood@redhat.com>
0f2772a to
5bfcacd
Compare
|
All three review comments have been addressed in the latest force-push:
|
|
There has been no activity on this PR for 45 days. What you can do:
/lifecycle stale |
|
There has been no activity on this PR for 59 days and it has been stale for 14 days. What you can do:
/lifecycle rotten |
|
There has been no activity on this PR for 66 days and it has been rotten for 7 days. What you can do:
/close |
|
@kubevirt-bot: Closed this PR. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/reopen |
|
@dhiller: Reopened this PR. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
|
||
| // OrgProjects holds all project definitions for a single GitHub organization. | ||
| type OrgProjects struct { | ||
| Projects map[string]Project `yaml:"projects"` |
There was a problem hiding this comment.
nit: Why do we need a map here? Wouldn't a slice be enough? Or has the key a specific use?
| }) | ||
| } | ||
|
|
||
| key := slugify(p.Title) |
There was a problem hiding this comment.
nit: if the key wasn't necessary we wouldn't need the func
There was a problem hiding this comment.
nit: wouldn't that rather belong inside config package?
Summary
pkg/ghprojectspackage androbots/ghprojectsCLI for managing GitHub Projects V2 boards declaratively from YAML configuration--confirmto apply, granular--fix-projects/--fix-fields/--fix-viewsflagsshurcooL/githubv4GraphQL library already vendored in this repodumpsubcommand to export live project state back to YAMLCloses #4814