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

Leverage the Checks API to report feedback about the outcome of applying settings changes #518

Open
travi opened this issue Nov 15, 2021 · 4 comments

Comments

@travi
Copy link
Member

travi commented Nov 15, 2021

I'm starting a new issue to gather the plans around this more cleanly than I've done in the past. I've let this important functionality slide up to this point and want to bring some priority back to it.

Goal

The most important feedback is showing errors so that users of the app are able to make adjustments to the settings.yml or other inputs in order to resolve the problems that are preventing their intended settings from being applied successfully. In addition to errors, it would be useful to report when the run is successful. It could also be helpful to report when a run is skipped because of no changes, but I'm unsure if it is worth the noise to report on all changes to the main branch.

Nice to Haves

These are open for discussion around whether they should be part of the initial iteration or added as a follow up. Also open to discussion around whether the complexity is worthwhile or if there are other considerations that i'm overlooking.

  • Should we report when the run is skipped because of no changes to the settings.yml?
  • Should we only report overall outcome, or separate checks for each plugin?
    • I lean toward this being valuable, but would this overly complicate handling of applying changes to the .github org-level file once that is handled better? I expect that we would want to report a single outcome for each child repo in that situation instead of each plugin for each child-repo.

Security Considerations

  • Can there be sensitive details in the API responses that we need to be careful not to disclose through these checks?
  • Can there be details included in API responses that are visible to the app because of it's permission level that should not be revealed to members with visibility to the checks that dont have the permissions level equivalent to the app?
  • Are there any similar considerations for errors that result from other runtime problems that are not API responses?

Related Historical issues and Pull Requests

@travi travi pinned this issue Nov 15, 2021
@hongaar
Copy link

hongaar commented Feb 28, 2022

I'm currently working on integrating the plugins from this respository into a custom GitHub Action to sync organisation defaults.

For this purpose, it would have added value if the plugin logic would be separated from the Probot wrapper in order to support more use-cases, and I imagine the output of the plugins could be decoupled as well so we can redirect it either to stdout (for use with GitHub Actions) or to a check (for use with the Probot app).

I'll probably be doing some refactoring in a fork (https://github.com/exivity/settings) Would be happy to coordinate changes and contribute to these plans.

NB - I don't know if Probot itself has plans to support GitHub Actions natively?

Edit: TypeScript port is now located at https://github.com/exivity/actions/tree/main/sync-defaults/src/settings).

@mattwynne
Copy link
Contributor

mattwynne commented Apr 23, 2022

It seems to me like a bare minimum for this would be to use https://docs.github.com/en/rest/checks/runs to create a check run for any commit that modifies settings.yml with:

status: completed
conclusion: failure | success # depending on outcome of applying the settings
output: <some useful logging>

Would that be a good place to start, and we can iterate from there?

@travi
Copy link
Member Author

travi commented May 4, 2022

sorry for the delayed response @mattwynne. yes, i think that is the best path forward. one additional detail that i think is important for the initial version is that we should set the status to in_progress when starting to process updates and later completed after the updates are complete.

Should we report when the run is skipped because of no changes to the settings.yml?

i'm wondering if this is a decision that should be covered initially. i suppose we could defer until after the basics are in place, but i think this could also confirm that the triggers are working even when the app decides that there are no actions to take. maybe this is still a separate feature from reporting the results when the app does decide tha there are actions to take.

@travi
Copy link
Member Author

travi commented May 4, 2022

also, https://github.com/wip/app could be a good reference to understand some of the details of managing the check runs

GitHub
The WIP GitHub App. Contribute to wip/app development by creating an account on GitHub.

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

No branches or pull requests

3 participants