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: Add support for Github App Installation Authentication Closes #390 #414
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ParthaI Please see review comments, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ParthaI please take a look at the review comments. Thanks!!
github/connection_config.go
Outdated
Token *string `hcl:"token"` | ||
BaseURL *string `hcl:"base_url"` | ||
Token *string `hcl:"token,optional"` | ||
BaseURL *string `hcl:"base_url,optional"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should BaseURL
be optional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the BaseURL
should be an optional parameter. This is only required for the GitHub Enterprise account.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ParthaI I think all the config arguments should remove the optional
part.
github/utils.go
Outdated
if err != nil { | ||
panic(err) | ||
} | ||
itr, err := ghinstallation.NewKeyFromFile(http.DefaultTransport, ghAppId, ghInstallationId, privateKeyPath) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add in a function to check if privateKeypath exists or not? Or is that taken care of by ghinstallation.NewKeyFromFile
function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should not check if privateKeypath exists or not. It would be better to return the error directly that is returning from the ghinstallation.NewKeyFromFile
function.
The following error we are getting:
Error: rpc error: code = Internal desc = github: rpc error: code = Internal desc = hydrate function tableGitHubMyRepositoryList failed with panic Error occurred in 'connectV4()' during GitHub App Installation client creation could not read private key: open /Users/parthas/abc/def/testappsetting.2024-01-18.private-key.pem: no such file or directory
github/utils.go
Outdated
if githubAppId != "" && githubInstallationId != "" && privateKeyPath != "" && token == "" { | ||
ghAppId, err := strconv.ParseInt(githubAppId, 10, 64) | ||
if err != nil { | ||
panic(err) | ||
} | ||
ghInstallationId, err := strconv.ParseInt(githubInstallationId, 10, 64) | ||
if err != nil { | ||
panic(err) | ||
} | ||
itr, err := ghinstallation.NewKeyFromFile(http.DefaultTransport, ghAppId, ghInstallationId, privateKeyPath) | ||
if err != nil { | ||
panic("Error occurred in 'connectV4()' during GitHub App Installation client creation" + err.Error()) | ||
} | ||
transport = itr | ||
|
||
client = githubv4.NewClient(&http.Client{Transport: itr}) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please take a look at similar comments above?
Example query results
Results