-
Notifications
You must be signed in to change notification settings - Fork 488
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 github-app-auth-only to scala-steward CLI #2973
Comments
I think one of the main reasons is because Scala Steward uses the |
IIUC, scala-steward can already take Github app credentials and use them to list all repos available to the app. My thinking was that this change should just restrict that list to the intersection of (all repos available to the app) and (all repos in repos.md). |
But in that case the two inputs won't be doing the same thing. Scala-steward doesn't use the provided app/id/secret-key to authenticate GitHub calls (like creating a PR) but just for retrieving the list of repositories available to the App since On the other hand the |
I think I'm starting to understand... It sounds like the Github app ID and key are actually only intended to be an alternative to repos.md. And you still have to provide a token regardless. If that's correct, I think I can probably make a PR to just clarify that in the docs. Right now, that's not obvious at all IMHO. |
Yeah, that is correct.
That would be more than welcome 😊 |
Help strings are now updated. Thanks for the quick review. I think it still might make sense to add this
|
I'm not so sure, this is something people could just add to their |
I agree with this. Also, if a GitHub App API token can be retrieved before running Scala Steward in a separate program, that would keep Scala Steward simpler. Maybe it would also make more sense to move the GitHub App code we currently have into a separate program that calls the GitHub App API to create the |
I agree that if we don't want to pull more github-specific logic into scala-steward, then the current logic for determining repos from a Github App probably makes more sense to be outside of core scala-steward. Maybe the official action is a good place for it. Whether it's worthwhile to rip it out at this point, is up to you. I'm happy closing the issue now. I see where you're both coming from, and I think the updated CLI docs should help clarify this for other users. |
I'm currently experimenting with this idea at https://github.com/scala-steward-org/scala-steward/compare/topic/gh-app-facade. The gist of that GitHub App facade is scala-steward/modules/gh-app-facade/src/main/scala/org/scalasteward/ghappfacade/FacadeAlg.scala Lines 50 to 78 in e72ff42
|
Prior to this change, the `gitAskPass` program was called once on start-up and its output was cached and used for API calls to the forge while Git itself does not cache its output but calls it anytime a password is needed. This means if the output of `gitAskPass` changes during a Scala Steward run, the new password is only used for Git operations but not for forge API calls. With this change we now do the same as Git and call `gitAskPass` everytime the password is needed. This should make it easier to support GitHub Apps proper which require different access tokens during a run. See also #2973 (comment).
Prior to this change, the `gitAskPass` program was called once on start-up and its output was cached and used for API calls to the forge while Git itself does not cache its output but calls it anytime a password is needed. This means if the output of `gitAskPass` changes during a Scala Steward run, the new password is only used for Git operations but not for forge API calls. With this change we now do the same as Git and call `gitAskPass` everytime the password is needed. This should make it easier to support GitHub Apps proper which require different access tokens during a run. See also #2973 (comment).
Branching off of this discussion: #2822
I would like to port the
github-app-auth-only
flag and corresponding behavior from the scala-steward-action directly into scala-steward.Is there any reason this would be a bad idea?
The text was updated successfully, but these errors were encountered: