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

int64 for appID/installationID #27

Closed
dinvlad opened this issue Sep 27, 2019 · 6 comments · Fixed by #30
Closed

int64 for appID/installationID #27

dinvlad opened this issue Sep 27, 2019 · 6 comments · Fixed by #30
Assignees

Comments

@dinvlad
Copy link

dinvlad commented Sep 27, 2019

Hi @bradleyfalzon and @wlynch,

I noticed the discussion re int64 in google/go-github#597, however ghinstallation accepts appID/installationID as regular int for initialization.

What would be your recommendation if I'd like to initialize ghinstallation based on appID/installationID from github.InstallationEvent? Currently, we have to downcast int64int to make it compatible with ghinstallation...

Thanks!

@wlynch
Copy link
Collaborator

wlynch commented Sep 27, 2019

I'm in favor of bumping it to int64 to match the rest of go-github, especially since there have been reported cases where int has caused problems.

My one hesitation is that this would be a breaking change for anyone currently doing the int cast (which I suspect given the go-github types is quite a few).

@bradleyfalzon Breaking changes are something that comes up fairly frequently. One option would be to introduce Go modules to the library to enable semver so that users wanting stability can pin to a version. wdyt?

@bradleyfalzon
Copy link
Owner

I think we should make the change from int to int64 for the same reasons go-github chose to, and for keeping some consistency with go-github as this library is likely heavily used with it.

As for the breaking change, I'm fine with this, see discussion in #28 for my initial thoughts on how to get there.

@bradleyfalzon
Copy link
Owner

@dinvlad are you using Go modules in your project currently?

To answer your question, for now convert from int64 to int as you probably are already.

We'll release a breaking change and bring this library up to v1.0.0 so you won't need to do that anymore.

@bradleyfalzon bradleyfalzon self-assigned this Sep 28, 2019
@dinvlad
Copy link
Author

dinvlad commented Sep 28, 2019

@bradleyfalzon thanks, yes I do use 1.11 modules! To deploy it in Cloud Functions, I have to run go mod vendor though (because of peculiarities of private module resolution in GCF). Is that going to affect anything?

@bradleyfalzon
Copy link
Owner

It shouldn't, at least not what I know of. What's the relevant lines in go.mod and go.sum for ghinstallation?

@dinvlad
Copy link
Author

dinvlad commented Sep 28, 2019

Mine says require github.com/bradleyfalzon/ghinstallation v0.1.2 and

github.com/bradleyfalzon/ghinstallation v0.1.2 h1:9fdqVadlvEX/EUts5/aIGvx2ujKnGNIMcuCuUrM6s6Q=
github.com/bradleyfalzon/ghinstallation v0.1.2/go.mod h1:VQsLlCoNa54/CNXcc2DuCfNZrZxqQcyPeqKUugF/2h8=

wlynch added a commit to wlynch/ghinstallation that referenced this issue Oct 9, 2019
This change makes ghinstallation consistent with
github.com/google/go-github for App and Installation identifiers. See
google/go-github#597 for related discussion.

Fixes bradleyfalzon#27
wlynch added a commit to wlynch/ghinstallation that referenced this issue Oct 9, 2019
This change makes ghinstallation consistent with
`github.com/google/go-github` for App and Installation identifiers. See
google/go-github#597 for related discussion.

Fixes bradleyfalzon#27
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 a pull request may close this issue.

3 participants