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

Config option to show/hide a "bundle analysis is working message" when no bundle changes occur. #1485

Open
Adal3n3 opened this issue Mar 28, 2024 · 5 comments
Labels
Dev-Ready This means the UX is reviewed and ready for prioritization scheduling. in discovery The design, product, and specifications require refinement P1: will do priority 7-9

Comments

@Adal3n3
Copy link

Adal3n3 commented Mar 28, 2024

Problem to solve

Description

According to @nicholas-codecov this message is working as intended, so it's not a bug. We will receive this message if they run their CI and run a build step. The problem is that developers will be annoyed by receiving this unnecessarily/non-actionable message "Bundle size has no change ✅" on their PR comment, especially for those who push 15 times PRs in a day.

Slack 🧵: https://sentry.slack.com/archives/C060Y7J8420/p1711645978145129
Example: getsentry/sentry#67874 (comment)

Screenshot 2024-03-28 at 10 00 46 AM

Possible solution

See comment at the bottom of this issue

@Adal3n3 Adal3n3 added in discovery The design, product, and specifications require refinement and removed in discovery The design, product, and specifications require refinement labels Mar 28, 2024
@Adal3n3
Copy link
Author

Adal3n3 commented Mar 28, 2024

@trent-codecov @rohan-at-sentry Can you prioritize this?

@eliatcodecov
Copy link

I've been reviewing this issue, and while I agree with the idea that not everyone wants to see a PR comment when "nothing changes", I really think this should be a configuration level change. I'll lay out some justification for why I feel this way:

  1. Codecov's PR comment has this same behavior. If you make a PR that doesn't impact coverage, we still tell you that coverage isn't impacted. So I think "no changes, everything's fine" PR comment has some precedence from our other features
  2. More importantly, though, as an engineer if I'm interacting with a service (in this case by sending a bundle) and I receive no response from the service, my first expectation is that it is flaky or down.
  3. Depending on context, it's not necessarily intuitive to me that the bundle size didn't change:
    • In some cases, the fact that the bundle didn't change is really easy to infer. In Sentry's case for example, if I have a PR that only changes the backend, any PR comment about bundle size is probably useless to me. There's no way I could've impacted the bundle to begin with.
    • What about cases where it isn't so easy to infer that my bundle shouldn't have changed? For example, I've made a net zero change to my bundle size but I've still made changes. In this case it would definitely help me to receive the PR comment in question.

So I think this PR comment is conditionally helpful, and when something is conditionally helpful it's typically worth leveraging configuration to let users decide if they want it or not. I'm not even super picky about what the default is, but I do think this needs to be a configuration setting.

Can we instead tackle this problem from a configuration approach?

@aj-codecov
Copy link

Agreed with Eli in this case, I think we can use the same yaml config that we have used for Coverage to only show when there's a change:

comment:
require_changes: true

Additionally I think we can alter the language of the no change message that we're currently showing to specify why it's there - ex. "Your most recent bundler run showed no change in bundle size ✅ "
Doesn't have to be that, but something similar would be positive to better contextualize the message.

@rohan-at-sentry
Copy link

rohan-at-sentry commented Mar 28, 2024

^ I think that's a good approach to consider. Throwing my 🎩 into this ring, there's feedback on only showing PR comments when "a thing" has materially changed - materially being config based.

Here's a link to that for coverage - #1350

There's an opportunity to take a first principles thinking approach into how we do comments overall

I think (in the order of weeks) I'd like for us to solve this specific BA issue - since this is PR related, we can tackle that on the platform team..

We can

  • leverage a yaml config and default to false, AND
  • update copy to show a comment if there were no bundle size changes (in order to mitigate the "is this working" concern)

and revisit more fundamental approach to all this later with @Adal3n3 and @codecovdesign

@codecovdesign codecovdesign added the in discovery The design, product, and specifications require refinement label Apr 16, 2024
@rohan-at-sentry rohan-at-sentry changed the title Remove "Bundle size has no change ✅" PR message Config option to show/hide a "bundle analysis is working message" when no bundle changes occur. Apr 16, 2024
@rohan-at-sentry
Copy link

rohan-at-sentry commented Apr 24, 2024

Summarizing above convo and setting out path forward

  • Add a new config to the yaml
    • require_bundle_changes : true / false (false is default)
  • Should be the under the comment config (which is used to modify the behavior of the coverage PR comment). Therefore, the following should be a valid config
comment:
require_changes:true
require_bundle_changes:true

IF require_bundle_changes is false, on commits/PRs where bundle size has not changed, then users see

image

This is the behavior today, and is going to be the default moving forward.

IF require_bundle_changes is true, on commits/PRs where bundle size has not changed, then users don't see the bundle PR comment from Codecov

Add documentation as necessary under https://docs.codecov.com/docs/javascript-bundle-analysis

@rohan-at-sentry rohan-at-sentry added the Dev-Ready This means the UX is reviewed and ready for prioritization scheduling. label Apr 24, 2024
@rohan-at-sentry rohan-at-sentry added this to the Q2'24 milestone Apr 24, 2024
@rohan-at-sentry rohan-at-sentry added the P1: will do priority 7-9 label Apr 24, 2024
@rohan-at-sentry rohan-at-sentry removed this from the Q2'24 milestone Apr 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Dev-Ready This means the UX is reviewed and ready for prioritization scheduling. in discovery The design, product, and specifications require refinement P1: will do priority 7-9
Projects
None yet
Development

No branches or pull requests

5 participants