Skip to content

Normalize forge statuses#121

Open
abhinavgautam01 wants to merge 3 commits into
git-pkgs:mainfrom
abhinavgautam01:feat/normalize-statuses
Open

Normalize forge statuses#121
abhinavgautam01 wants to merge 3 commits into
git-pkgs:mainfrom
abhinavgautam01:feat/normalize-statuses

Conversation

@abhinavgautam01

Copy link
Copy Markdown

closes #17

Summary

Adds shared normalized status enum types across forge implementations.

This includes normalized types for:

  • issue states
  • pull request statuses
  • CI run/job statuses and conclusions
  • commit status states
  • collaborator access levels

Also updates GitHub, GitLab, Gitea, and Bitbucket adapters to map forge-specific values into the shared enums, so callers do not need to handle each forge’s raw status names.

Testing

env GOCACHE=/private/tmp/forge-gocache go test ./...
git diff --check

@andrew andrew left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice implementation of #17. The Normalize* helpers are well tested, the ~string generic on assertEqual keeps the adapter tests tidy, and the reverse mappers (githubPermission, giteaPermission, the GitLab fallback in parseGitLabAccessLevel) mean --permission write now works against every forge, which is a real usability win.

A few mapping gaps to close before merging, mainly the Bitbucket issue-state regression. See inline.

Minor naming thought: IssueState but PRStatus, with the struct field still called State and the JSON key "state". PRState would be more consistent, but not worth churning if you'd rather keep it.

Comment thread bitbucket/issues.go
Comment thread statuses.go Outdated
Comment thread statuses.go
Comment thread types.go Outdated
Comment thread gitlab/ci.go Outdated
result := forge.CIRun{
ID: int64(p.ID),
Status: p.Status,
Status: forge.NormalizeCIStatus(p.Status),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not blocking, just noting: GitLab merges status and conclusion into one field, so a successful pipeline ends up Status: success, Conclusion: "" while GitHub is Status: completed, Conclusion: success. Callers still have to check both. Worth a follow-up to populate Conclusion from terminal GitLab statuses now that the values are normalized.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added logic to explicitly populate Conclusion for GitLab pipelines and jobs if they've reached a terminal state.

Comment thread internal/cli/status.go Outdated

@andrew andrew left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, all addressed. One small ask: add the new cases (on_hold, invalid, duplicate, wontfix, triage, maintain, planner) to the table in statuses_test.go so the Bitbucket regression stays covered. Happy to merge once that's in.

@andrew andrew left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, let's hold merge until the new mappings are covered: add on_hold, invalid, duplicate, wontfix, triage, maintain, planner to the tables in statuses_test.go. Everything else from the first round is addressed.

@abhinavgautam01

Copy link
Copy Markdown
Author

Thanks @andrew
I've added the missing issue cases ( on_hold , invalid , duplicate , wontfix ) to a new TestNormalizeIssueState table and the missing access levels ( triage , maintain , planner ) to the TestNormalizeAccessLevel table in statuses_test.go to ensure the Bitbucket regression stays covered. Tests and formatting are passing locally.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Normalize statuses across forges

2 participants