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

Add default GitHub API client #501

Merged
merged 4 commits into from
Jan 31, 2021

Conversation

ganboonhong
Copy link
Contributor

I want to access the migration source from GitHub without providing personal access token.

Is it possible to add such a feature since go-github package allows unauthenticated clients to call the API?

source/github/github.go Outdated Show resolved Hide resolved
source/github/github.go Outdated Show resolved Hide resolved
Copy link
Member

@dhui dhui left a comment

Choose a reason for hiding this comment

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

Just one minor nit

return nil, ErrNoUserInfo
}
// use http.DefaultClient
client = nil
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't client default to nil? Invert the condition and don't bother with an else branch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

g := &Github{}
owner := "golang-migrate"
repo := "migrate"
path := "source/github/examples/migrations"
Copy link
Member

Choose a reason for hiding this comment

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

Nice! Thanks for adding tests!

return nil, ErrNoUserInfo
}
// client defaults to http.DefaultClient
client = nil
Copy link
Member

Choose a reason for hiding this comment

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

Replace this with var client *http.Client. You don't need to set the pointer to nil since it'll default to nil.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice catch. Do you think // client defaults to http.DefaultClient comment is needed above var client *http.Client?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, let's leave that comment

Copy link
Member

@dhui dhui left a comment

Choose a reason for hiding this comment

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

Thanks for the PR and for addressing all of my feedback!

@dhui dhui merged commit dab829b into golang-migrate:master Jan 31, 2021
@lukasmrtvy
Copy link

@dhui hey 👋 is possible to release new version containing this change? thanks

@dhui
Copy link
Member

dhui commented Jul 6, 2021

Releases are still blocked, see: #546
This PR is the most promising to unblock releases, but I haven't had time to test it.

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.

3 participants