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

Rewrite CLI in Go #24

Merged
merged 13 commits into from
Nov 9, 2023
Merged

Rewrite CLI in Go #24

merged 13 commits into from
Nov 9, 2023

Conversation

lindluni
Copy link
Collaborator

@lindluni lindluni commented Sep 10, 2023

This PR introduces a full rewrite of the gh-token CLI as a pure-Go gh CLI extension.

Of note is the choice of urfave/cli over spf13/cobra for the command package. Having spent years using spf13/cobra I prefer urfave for the simplicity due to the future features this extension itself actually requires, the Cobra command package provides much more functionality than we need.

While I pulled in google/go-github, I chose to use it for the types only, and not for using it as an API client. Using the go-github client pulls in many 3rd party dependencies, including the additional 3rd party ghinstallations library. All of this just increases the maintenance surface, so I chose to implement the http calls using pure Go, which reduces the maintenance burden of keeping the app's dependencies up to date.

To test this PR you can build and install the extension locally, in prod the extension will be built and pinned by Actions as a release:

gh extension remove token
git clone [email protected]:Link-/gh-token
cd gh-token
gh pr checkout 24
go build .
gh extension install .

@lindluni lindluni force-pushed the rewrite branch 8 times, most recently from 1654ed5 to bf776cd Compare September 10, 2023 22:03
@github-advanced-security
Copy link

This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation.

@lindluni lindluni force-pushed the rewrite branch 9 times, most recently from a884a39 to 28ebbf7 Compare September 10, 2023 22:40
@lindluni lindluni marked this pull request as ready for review September 10, 2023 22:41
@lindluni lindluni linked an issue Sep 10, 2023 that may be closed by this pull request
@lindluni
Copy link
Collaborator Author

@Link-

I'll ping you on Monday. I'd like to get some app credentials into the Actions Secret store for this repo for a test app so I can write e2e integration tests for the functionality.

Eventually I'll get to writing unit tests as well, but the scope of the CLI is small, so integration tests will be a better use of my dev cycles.

Signed-off-by: Brett Logan <[email protected]>
@Link-
Copy link
Owner

Link- commented Sep 11, 2023

Fixes #24 - I'll review shortly

internal/generate_flags.go Outdated Show resolved Hide resolved
images/gh-token.png Outdated Show resolved Hide resolved
Copy link
Owner

@Link- Link- left a comment

Choose a reason for hiding this comment

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

This is great, I still need to run some tests and we need to do the following before merging:

  • Push out an announcement that we're migrating to Go
  • Update README.md to cover the new changes
  • It'll be awesome to have an e2e test workflow (but this is a nice to have)

@lindluni lindluni requested a review from Link- September 15, 2023 04:58
Signed-off-by: Brett Logan <[email protected]>
@lindluni
Copy link
Collaborator Author

lindluni commented Sep 15, 2023

This is great, I still need to run some tests and we need to do the following before merging:

  • Push out an announcement that we're migrating to Go
  • Update README.md to cover the new changes
  • It'll be awesome to have an e2e test workflow (but this is a nice to have)

I 💯 agree on the e2e tests, this should be simple to test all functions, I'll ping you in Slack with app credentials, if you can add them to the secret store I'll write the e2e tests tomorrow, I have some free cycles Friday afternoon.

@Link-
Copy link
Owner

Link- commented Sep 24, 2023

@lindluni can you have a look at the recent changes? I think we're ready to merge this when this deadline hits: https://github.com/Link-/gh-token/discussions/26

@lindluni
Copy link
Collaborator Author

Love it!

It'd still be great if we could get some unit tests and then some E2E tests done in the future with an actual app, but that's a story for another PR, and maybe something someone in the community would be interested in helping with (we can open a help-wanted issue after the merge).

I do think we should create a tag for the current main as v1.0.0 and then merge this and tag it as v2.0.0 latest, so anyone who still needs the old code can grab it from the release.

Copy link
Owner

@Link- Link- left a comment

Choose a reason for hiding this comment

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

Let's do this!

@Link- Link- merged commit 52f27bb into main Nov 9, 2023
3 checks passed
@Link- Link- deleted the rewrite branch November 26, 2023 11:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
✨ feature New feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Full re-write in Go
2 participants