Ability to approve the changes to the bundle size

[markwhitfeld]

Is your feature request related to a problem? Please describe.
Firstly, thank you so much for this awesome tool!!!
I have gone through a few similar tools and this is definitely exactly what I needed.
The issue I am facing:

• when a PR fails the bundle checks, then the check is marked as failed for the PR (which is obviously the correct behavior)
• if this is an optional check then, as a reviewer, even though it has failed I can approve and merge the PR
• BUT now, the check fails on the master/main branch, which makes it look like the build is failing
• Also, it would be great to be able to mark this check as passing in a PR so that it does not block the PR if it is a required check.

Describe the solution you'd like
Potentially some sort of "Approve" or "Accept" facility on the check (or on the page that it leads to) that would allow for the status of
the check to no longer be marked as failing. This would be very useful for the case that I mentioned for a master commit, and for reviewing a PR.

I guess that this would only be available to a CODEOWNER or a moderator of the target repository.
Hopefully this auth aspect doesn't make things too difficult. One potential simple option here could be a configured approver list in the github app config, or some sort of secret key that could be shared with the relevant parties and they just copy paste after clicking the approve button.

I have seen this type of thing in CodeClimate: https://docs.codeclimate.com/docs/workflow#what-to-do-when-issues-are-found

Describe alternatives you've considered
I tried to use the github actions continue-on-error feature with the approvals feature to facilitate this workflow, but the check still marks the build as failing. (See: ngxs/store#1886)

Potentially also the "Approve" could be done using a comment in the PR by a moderator using the comment /bundlemon approve, but this doesn't solve the issue with the commit on master.

Additional context
Here is an example of a commit on master that fails after merging a PR with a bundle size change outside of the tolerances:
ngxs/store@80a3014

[LironEr]

Hey,

BUT now, the check fails on the master/main branch, which makes it look like the build is failing

If the target branch doesn't exist (meaning we are not in PR), BundleMon will compare your current commit to a previous commit in the same branch you currently are. This was implemented to show the correct message at the end of the run and in the commit status.
But I think you are right and there is no need to fail the run if we are comparing commits in the same branch.

What do you think about adding a new flag to ignore fails in the same branch?

ignoreFailuresInSameBranch: true / false (default true)

The flag will only change the result to success

Also, it would be great to be able to mark this check as passing in a PR so that it does not block the PR if it is a required check.

This is a really cool idea, I will need to do some investigation and think about how to do that, your references are great and I will check them.

Thank you for your suggestions (:

[markwhitfeld]

Super! Thank you so much for the response.
Regarding the master branch, I don't know what is possible with checks, but potentially they could be represented as warnings and not failures?
And for the setting name, I think that the terminology could potentially be "base" branch, because some might interpret commits within the PR branch as the "same" branch.
Potentially the setting could be called:
warnForFailuresOnBaseBranch: true/false
Potentially making the default true might be an unexpected behaviour change (currently it would behave as if it was set to false). But it is up to you if you see the current failure on the base branch as a bug or a feature, so it is up to you 👍

Just jotting down the progression of my thoughts here.....

An alternative setting name could be:
baseBranchFailureHandling: 'error'/'warn'/'ignore'
or baseBranchFailureStrategy: 'error'/'warn'/'ignore'
or baseBranchCommitValidation: 'error'/'warn'/'ignore'

or even ignoreFailuresOnBranches: [ 'master', 'main', 'release' ]
or warnOnlyForFailureOnBranches: [ 'master', 'main', 'release' ]
or softFailureOnBranches: [ 'master', 'main', 'release' ]

or even this could be in the github reporter options like this

"reportOutput": [
  [
    "github",
    {
      "checkRun": false,
      "commitStatus": true,
      "prComment": true,
      "branchCheckHandling": {
          "master": { "reportFailureAs": "warning" }, 
          "dev": { "reportFailureAs": "error" }, 
          "release": { "reportFailureAs": "information" }, 
      }
    }
  ]
]

I think I like the configurable aspect of the last one. Obviously whether or not we can do warnings would affect the design.

Haha, really over-engineering now, Imagine if we could even have "reportFailureAs": [ "warning", "email" ], which would email the code owners. Hahaha, I think there is some serious scope creep here 🤣 .

[markwhitfeld]

PS. This might be the simplest of the ideas around approving a bundle change in a PR:

Potentially also the "Approve" could be done using a comment in the PR by a moderator using the comment /bundlemon approve...

It would also, as a side effect, show previous approvals in the PR comment history, and would only apply the approval to the check immediately prior to the comment in the PR.
Hopefully github has some way to find out who the moderators/codeowners are for a PR.
And If I let my mind run with this a bit, it could be an option to require 1, 2, or all moderators to approve.

[tleunen]

In addition to what @markwhitfeld is asking, I think it would also be great to have a way to report the output differently in case of "failures".
For example, the report as PR comment can be very noisy in a monorepo with multiple projects reporting as PR comments, but if we can set the PR comment to be only in case of "failure", that would already be a lot less noisy.

That would also play very nicely with the approval comment /bundlemon approve since we will be getting the comment history (failure/approval) for free.

[LironEr]

I created separate issues to keep this one only about the "approve changes" feature (:

[LironEr]

I started to look for a possible solution.

With the current BundleMon GitHub App permissions I can fetch all PR comments, each comment has author_association property:

COLLABORATOR

Author has been invited to collaborate on the repository.

CONTRIBUTOR

Author has previously committed to the repository.

FIRST_TIMER

Author has not previously committed to GitHub.

FIRST_TIME_CONTRIBUTOR

Author has not previously committed to the repository.

MANNEQUIN

Author is a placeholder for an unclaimed user.

MEMBER

Author is a member of the organization that owns the repository.

NONE

Author has no association with the repository.

OWNER

Author is the owner of the repository.

https://docs.github.com/en/graphql/reference/enums#commentauthorassociation

So if I detect a comment with something like /bundlemon approve and the comment author_association is one of COLLABORATOR / MEMBER / OWNER than mark the status as a success

What do you think?

[LironEr]

Another solution is to look for a label in a PR
I think only maintainers can add labels to PRs

[tleunen]

I think an approval from anyone with write access to the repo is good enough, indeed :)
If we really want to be extra picky, we can disallow the comment from the PR owner, but I'd rather have this as an extra option imo as it can be more complicated in small teams/repos.

[markwhitfeld]

According to the github docs, anyone with Triage access can apply or dismiss labels:
https://docs.github.com/en/issues/using-labels-and-milestones-to-track-work/managing-labels#applying-a-label

This could be a reasonable mechanism to achieve what we are looking for. Bundlemon would need to dismiss any approval labels if there is a subsequent commit that causes another failure. Then the approver would have to apply the "Approved Bundle Size" (or similar) label again to re-approve.

That being said, I think that the comment mechanism might provide more control.
I agree that any user from COLLABORATOR / MEMBER / OWNER should be able to approve.

With respect to limiting the PR Author, I don't think that restriction is necessary if the PR Owner already has one of the write access permissions. In that case, if the organisation is looking for control over PRs then they would enable the setting that there needs to be a separate approver of the PR. That approver would be responsible for checking that the failure has not needlessly been ignored by the PR Author. Using comments does also make this action more visible.

PS. on a side note, what happens if someone adds an approve comment and then deletes it. Then there is no trace of the approval. Maybe the deletion of an applicable /bundlemon approve comment would revert back to the state of the check that would be in place if the comment was not there.

[LironEr]

PS. on a side note, what happens if someone adds an approve comment and then deletes it. Then there is no trace of the approval. Maybe the deletion of an applicable /bundlemon approve comment would revert back to the state of the check that would be in place if the comment was not there.

We would also want to only be looking at comments that were created after the last commit was pushed to the PR (or since the most recent Bundlemon run for the PR).
#131 (comment)

Currently, I search for the approve comment when creating Github output, so only after you get a failed status you will need to add a comment and rerun the job or create a new commit. So I'm not sure I can make something that will last only for one commit.

ideally, I would use webhooks to get updates on the comments, but because I'm using a free server I can't handle a lot of requests right now.

Maybe a better solution, for now, is to add a GitHub action that gets triggered by adding/removing a comment if the comment starts with /bundlemon send a request to bundlemon server and handle it.
In that way, we can change the status to success for only one commit and also change the status to failed in case of deletion.

The downside of this solution is that you need to create a workflow just to use this feature.

[markwhitfeld]

I honestly think that requiring people to create a workflow is a great option. It gives you less to maintain and provides more flexibility for the user to implement what they want.
There could be an example implementation in the docs.
I'm thinking that we would just need a way to update the bundle size status for a specific commit.
I'm thinking, that in response to an approve comment, the user could call something like:

      - name: Update BundleMon Status
        run: yarn bundlemon approve
        env:
          BUNDLEMON_PROJECT_ID: 62b174ff6619780d8caf79fa
          CI_COMMIT_SHA: ${{ github.event.pull_request.head.sha }}

and when the approve comment is deleted, you would call:

      - name: Reset BundleMon Status
        run: yarn bundlemon reset
        env:
          BUNDLEMON_PROJECT_ID: 62b174ff6619780d8caf79fa
          CI_COMMIT_SHA: ${{ github.event.pull_request.head.sha }}

The second one would reset the bundlemon status back to whatever the original result of the commit run was.

I'm not sure if you would have access to that commit's status for the reset, so if not then conceivably it could be achieved by triggering the bundlemon command to run again for that commit, thereby running the check again.

[markwhitfeld]

PS. I'm wondering what the security of exposing a command like this would be. For example if someone just copies those keys and runs it from their local machine then they can force the approval. I don't know if there is anything else that you can do to check permissions when the command is run.
On the other hand, there is really no benefit to a malicious actor like this, is there?

[tleunen]

Using this workflow I'd assume. Having a workflow to approve Bundlemon could be valid. The security concern would be limited as you'd still require the api key for such commands.

But overall, I think that if you use the BundleMon github app already, this should be set automatically by the app instead of having a custom workflow on top of it.

[LironEr]

Feature available in bundlemon@2.0.0-rc.1

[markwhitfeld]

Fantastic! Thank you so much!

[markwhitfeld]

Is there an approve/reset command line option available now?
(like what was described in this comment)

[LironEr]

Oh sorry, I forgot to write about the new solution.

I started implementing it as a GitHub workflow, but I had issues with how to handle editing or deleting comments that had a command in them.

So I went in a bit different direction, to approve a limit:

1. Go to the BundleMon website
2. Login with GitHub (same permissions as the BundleMon GitHub app)
3. Approve the report

approve-commit-record.mp4

It's more stuff that the user needs to do if he wants to approve, but it makes the installation process of BundleMon easier.

For a long time, I wanted to implement user login with GitHub, so I figured it was a good opportunity. It opens more options like showing all the user's repositories in BundleMon, some statistics, project settings, and more...

Currently, I didn't implement disapprove, but I will do that soon. #141

What do you think? (:

[markwhitfeld]

I think that that is a good solution. Well done and thank you!
Maybe one thought is on the possible actions that can be taken:

• approve - mark the check as passing, even if one of the sizes are failing
• reject - mark the check as failing, even if all of the sizes are passing (I think that you are referring to this as 'disapprove' at the moment? Not sure the use case here if that is what is meant.)
• reset - mark the check in accordance to the overall pass/failure of the sizes (back to the default status)

Potentially these could also cause Bundlemon to post a message on the PR (if it is a PR) to state the explicit action taken for the record.

PS. I do think that it would be valuable to have each of these actions invocable from the command line too in case the user would like to integrate some sort of other workflow (like using the PR comments). Then that complexity would be entirely for them to handle and not your lib.

[LironEr]

The current "disapprove" is to undo your approve action.

I love your suggestion to separate it to approve, reject and reset.

Also, I agree that it would be good to have these actions in the CLI, then you will probably need to use the project API key

[LironEr]

I started to work on approve, reject and reset, probably will be able to release it in a few hours

review.mp4

For simplicity the final report status will only check the last review, probably later we can add multiple approvers and stuff like that...

[tleunen]

This s fantastic @LironEr. Thank you so much for all of this. I'll be testing it real soon :)
It's great to have the history of approval/reject/reset as well.

[tleunen]

Can you describe what permissions we need to give when generating a github token for Bundlemon? Could be interesting to add this to the readme file

[LironEr]

You will need to generate access token with repo:* permissions

I will add it to the readme file.

I'm testing a way to generate a token without any permission, just to verify the user and his relation to the repo, then use the Github app to create outputs, but I have security concerns.. So if anything will change I will post about it.

[tleunen]

I find reject a bit strange maybe. Why would someone mark a report to fail especially, if the limit has not been reached?

Can I also ask the github comments to contain the history of approvals/rejects so it's easy to see without going on the report page?