Skip to content
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(manager): add GitManager #1268

Merged
merged 18 commits into from
Feb 2, 2024
Merged

feat(manager): add GitManager #1268

merged 18 commits into from
Feb 2, 2024

Conversation

angeloashmore
Copy link
Member

@angeloashmore angeloashmore commented Jan 18, 2024

Context

DT-1891

The Solution

Add GitManager (accessible via manager.git) with the following methods:

  • createGitHubAuthState()
  • fetchOwners()
  • fetchRepos()
  • fetchLinkedRepos()
  • linkRepo()
  • unlinkRepo()
  • checkHasWriteAPIToken()
  • updateWriteAPIToken()
  • deleteWriteAPIToken()

The tests introduced in this PR cover ~80% of the code, which is sufficient for a first release.

Impact / Dependencies

This PR allows us to implement the Git integration UI.

This PR also introduces a drastically simpler method for writing manager tests (inspired by @xrutayisire's Playwright setup). Only the new GitManager tests use the new method, but we can migrate existing tests to the new method over time.

Checklist before requesting a review

  • I hereby declare my code ready for review.
  • If it is a critical feature, I have added tests.
  • The CI is successful.
  • If there could backward compatibility issues, it has been discussed and planned.

Copy link

linear bot commented Jan 18, 2024

Copy link

vercel bot commented Jan 18, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
slice-machine ✅ Ready (Inspect) Visit Preview Feb 1, 2024 10:28pm

Copy link
Collaborator

@bapmrl bapmrl left a comment

Choose a reason for hiding this comment

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

Excellent work! I really like the new createAPIFixture function.

packages/manager/src/managers/git/GitManager.ts Outdated Show resolved Hide resolved
packages/manager/src/managers/git/GitManager.ts Outdated Show resolved Hide resolved
packages/manager/src/managers/git/types.ts Show resolved Hide resolved
packages/manager/src/managers/git/GitManager.ts Outdated Show resolved Hide resolved
packages/manager/src/managers/git/GitManager.ts Outdated Show resolved Hide resolved
packages/manager/test/__testutils__/createAPIFixture.ts Outdated Show resolved Hide resolved
packages/manager/test/__setup__.ts Outdated Show resolved Hide resolved
Copy link
Collaborator

@bapmrl bapmrl left a comment

Choose a reason for hiding this comment

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

I'm approving 😊 There's just a small conversation remaining. You can add a comment for the null value if you also think it's useful.

@angeloashmore angeloashmore marked this pull request as ready for review February 1, 2024 23:54
@angeloashmore angeloashmore merged commit fda3f09 into main Feb 2, 2024
34 of 35 checks passed
@angeloashmore angeloashmore deleted the aa/dt-1891 branch February 2, 2024 00:16
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.

2 participants