opensearch-project / .github

Provides templates and resources for other OpenSearch project repositories.
Apache License 2.0
29 stars 71 forks source link

Adds a check on pull requests for baking time #141

Closed peternied closed 1 year ago

peternied commented 1 year ago

Description

After pull requests are updated, a 2 day waiting period is required to ensure plenty of time for additional reviews and comments.

This repository handles changes that are sensitive to the OpenSearch-Project involving many maintainers/contributors by using a tool to enforce the 'bake time' it removes maintainers from having to remember how long to wait and communicates to contributors when a pull request is ready to merge.

The underlying github actions for this change are in https://github.com/peternied/bake-time to make this functionality reusable in other repositories.

Issues

Additional Background

Problem

This .github repository represents OpenSearch-Project's policies, ideals, and intentions. This repository has no quality gates other than 2 approvals and a DCO check - making the way pull requests are accepted subjective based on which maintainers/contributors first responded.

Expectation

There should be mechanisms to ensure a minimum quality of the changes that are accepted into this repository.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license. For more information on following Developer Certificate of Origin and signing off your commits, please check here.

peternied commented 1 year ago

While troubleshooting the permissions issue, I'm would appreciate any feedback on the concept.

dblock commented 1 year ago

I really like this idea!

From the implementation POV I think you should combine start and finish actions into a single pass GHA that is smart enough to know what it's running against (PR/cron). As a user, I'd like to write a single workflow.

name: Bake Time

on:
  workflow_dispatch:
  pull_request:
    types:
      - opened
      - synchronize
  schedule:
    - cron: '0 */1 * * *' # finish/start baking every hour

jobs:
  bake:
    runs-on: ubuntu-latest
    steps:
      - uses: peternied/bake-time/bake@v1
        with:
          github-token: ${{ secrets.GITHUB_TOKEN }}
          bake-time: 48
peternied commented 1 year ago

single pass GHA that is smart enough to know what it's running against (PR/cron)

Great idea!

dblock commented 1 year ago

This looks great! Is there any way to change the GHA to report pending status rather than a failing status? And even say how much time is left to wait / when the last check ran?

Reading https://dev.to/cardinalby/scheduling-delayed-github-action-12a6 it seems like you should be able to change the cron into a trigger that runs once, then dispatches itself/another job with a wait timer (https://docs.github.com/en/actions/deployment/targeting-different-environments/using-environments-for-deployment#wait-timer) and repeats it until it runs out of time.

peternied commented 1 year ago

Is there any way to change the GHA to report pending status rather than a failing status?

When using the pull request trigger via GitHub Actions, no. Yes - by using a GitHub bot/webhook.

This is a trade-off as this version will works with the fewest permissions being in play during a pull request and visibility in the GitHub action script/logs.

And even say how much time is left to wait / when the last check ran?

Nice idea, the scheduled action could update a hours remaining value and make it displaying pending instead of failed

Representing the 'last ran time' is difficult with static text, it would need to be in UTC otherwise we run into timezone problems, otherwise it would say 'last run an hour ago' or something less useful

cron into a trigger that runs once

Nice find, we would need a personal access token to perform the workflow dispatch, looks like it would only invoke the work once when the bake time was over - possible if we built up a bot/webhook.

peternied commented 1 year ago

Is there any way to change the GHA to report pending status rather than a failing status?

Nice idea, the scheduled action could update a hours remaining value and make it displaying pending instead of failed

Following up on this, the check cannot have its status modified from 'completed' -> 'pending'. so the UX does not look any cleaner 😞

And even say how much time is left to wait / when the last check ran?

Done, example: image

@dblock @CEHENKLE @mch2 @saratvemulapalli What are your thoughts on merging this change?

dblock commented 1 year ago

I like that it says "2 hours remain". I am ok with this. Will you release a version of the bake time GHA first to marketplace so we can reference something stable and open feature requests for the other asks?

peternied commented 1 year ago

Great idea, I have published 'bake-time' in the marketplace - as always feel to file issues and/or make pull requests

https://github.com/marketplace/actions/bake-time

saratvemulapalli commented 1 year ago

I still didnt really understand the intent, maintainers of repo do this anyway right? Is this a mechanism to enforce it?

peternied commented 1 year ago

@saratvemulapalli Thanks - created an issue to capture the problem

maintainers of repo do this anyway right?

The way I tend to maintain repositories is to try to merge changes as quickly as possible or provide feedback so that on the next iteration the change can be merged. I am a fan of frequent smaller PRs. When I see a change passes all checks and I merge it, even if I approved it a second ago.

peternied commented 1 year ago

Looks like I didn't push the tag after first creating the pushing update to my branch. I've re-run the workflow and its failed as expected. image

dblock commented 1 year ago

The last commit was more than 48 hours ago. Is it baking post last comment?

peternied commented 1 year ago

This background job that marks the 'Bake time / Baking pull request...' complete is not running until after this change has been merged. So this PR will need to be merged even with a failing bake time check (Irony!).

dblock commented 1 year ago

@peternied Should I override branch protection and merge this?

peternied commented 1 year ago

Should I override branch protection and merge this?

Please do!