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

Remove non link brackets from release notes #5737

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

daymxn
Copy link
Member

@daymxn daymxn commented Feb 23, 2024

Per b/326629027,

This adds a transformation in our release notes to separate brackets from their content if they're not wrapping a link. This is to avoid propagating the content to the release notes, as it shouldn't be there. In the future, we may want to add some warning for instances found (as they may have been intended to be links).

@daymxn daymxn self-assigned this Feb 23, 2024
Copy link
Contributor

github-actions bot commented Feb 23, 2024

📝 PRs merging into main branch

Our main branch should always be in a releasable state. If you are working on a larger change, or if you don't want this change to see the light of the day just yet, consider using a feature branch first, and only merge into the main branch when the code complete and ready to be released.

Copy link
Contributor

buildSrc Test Results

37 tests   37 ✔️  1m 18s ⏱️
  8 suites    0 💤
  8 files      0

Results for commit d009d78.

@daymxn daymxn enabled auto-merge (squash) February 23, 2024 20:33
@google-oss-bot
Copy link
Contributor

@google-oss-bot
Copy link
Contributor

Coverage Report 1

Affected Products

  • firebase-appdistribution

    Overall coverage changed from ? (2865f2b) to 75.99% (e77deca) by ?.

    68 individual files with coverage change

    FilenameBase (2865f2b)Merge (e77deca)Diff
    AabUpdater.java?98.36%?
    AabUpdater_Factory.java?0.00%?
    ApkInstaller.java?100.00%?
    ApkInstaller_Factory.java?0.00%?
    ApkUpdater.java?92.63%?
    ApkUpdater_Factory.java?0.00%?
    AppDistributionReleaseImpl.java?100.00%?
    AppDistributionReleaseInternal.java?100.00%?
    AppDistroComponent.java?0.00%?
    AppDistroComponent_MainModule_BindContentResolverFactory.java?0.00%?
    AppIconSource.java?84.62%?
    AppIconSource_Factory.java?100.00%?
    AutoValue_AppDistributionReleaseImpl.java?65.45%?
    AutoValue_AppDistributionReleaseInternal.java?71.58%?
    AutoValue_ImageUtils_ImageSize.java?35.00%?
    AutoValue_TesterApiDisabledErrorDetails.java?29.41%?
    AutoValue_TesterApiDisabledErrorDetails_HelpLink.java?54.17%?
    AutoValue_UpdateProgressImpl.java?65.96%?
    DaggerAppDistroComponent.java?80.56%?
    DevModeDetector.java?9.09%?
    DevModeDetector_Factory.java?100.00%?
    ErrorMessages.java?0.00%?
    FeedbackActivity.java?3.39%?
    FeedbackActivity_MembersInjector.java?0.00%?
    FeedbackSender.java?84.48%?
    FeedbackSender_Factory.java?0.00%?
    FeedbackTrigger.java?61.54%?
    FirebaseAppDistributionExceptions.java?80.00%?
    FirebaseAppDistributionFileProvider.java?0.00%?
    FirebaseAppDistributionImpl.java?89.89%?
    FirebaseAppDistributionImpl_Factory.java?0.00%?
    FirebaseAppDistributionLifecycleNotifier.java?91.49%?
    FirebaseAppDistributionLifecycleNotifier_Factory.java?0.00%?
    FirebaseAppDistributionNotificationsManager.java?88.89%?
    FirebaseAppDistributionNotificationsManager_Factory.java?0.00%?
    FirebaseAppDistributionRegistrar.java?95.83%?
    FirebaseAppDistributionTesterApiClient.java?88.78%?
    FirebaseAppDistributionTesterApiClient_Factory.java?0.00%?
    HttpsUrlConnectionFactory.java?50.00%?
    HttpsUrlConnectionFactory_Factory.java?100.00%?
    ImageUtils.java?100.00%?
    InstallActivity.java?2.67%?
    LogWrapper.java?86.67%?
    NewReleaseFetcher.java?86.67%?
    NewReleaseFetcher_Factory.java?0.00%?
    PackageInfoUtils.java?42.86%?
    ReleaseIdentifier.java?91.78%?
    ReleaseIdentifier_Factory.java?0.00%?
    ReleaseUtils.java?83.33%?
    ScreenshotTaker.java?36.17%?
    ScreenshotTaker_Factory.java?0.00%?
    SequentialReference.java?100.00%?
    SignInResultActivity.java?0.00%?
    SignInStorage.java?100.00%?
    SignInStorage_Factory.java?0.00%?
    TakeScreenshotAndStartFeedbackActivity.java?0.00%?
    TakeScreenshotAndStartFeedbackActivity_MembersInjector.java?0.00%?
    TaskCache.java?100.00%?
    TaskCompletionSourceCache.java?72.41%?
    TaskUtils.java?77.50%?
    TesterApiDisabledErrorDetails.java?93.75%?
    TesterApiHttpClient.java?90.09%?
    TesterApiHttpClient_Factory.java?0.00%?
    TesterSignInManager.java?95.45%?
    TesterSignInManager_Factory.java?0.00%?
    UpdateProgressImpl.java?100.00%?
    UpdateTaskCache.java?91.30%?
    UpdateTaskImpl.java?76.32%?

  • firebase-database

    Overall coverage changed from 50.17% (2865f2b) to 50.22% (e77deca) by +0.05%.

    FilenameBase (2865f2b)Merge (e77deca)Diff
    ChildChangeAccumulator.java83.33%96.67%+13.33%
    QueryParams.java89.76%90.36%+0.60%
  • firebase-firestore

    Overall coverage changed from 44.86% (2865f2b) to 44.85% (e77deca) by -0.00%.

    FilenameBase (2865f2b)Merge (e77deca)Diff
    SetMutation.java97.22%94.44%-2.78%
  • firebase-messaging

    Overall coverage changed from 83.97% (2865f2b) to 83.93% (e77deca) by -0.04%.

    FilenameBase (2865f2b)Merge (e77deca)Diff
    FirebaseMessaging.java73.13%73.57%+0.44%
    RequestDeduplicator.java90.00%80.00%-10.00%
  • firebase-storage

    Overall coverage changed from 83.92% (2865f2b) to 83.96% (e77deca) by +0.04%.

    FilenameBase (2865f2b)Merge (e77deca)Diff
    StreamDownloadTask.java88.41%88.89%+0.48%

Test Logs

  1. https://storage.googleapis.com/firebase-sdk-metric-reports/Todg8XfTNP.html

@google-oss-bot
Copy link
Contributor

Startup Time Report 1

The report is too large (122,698 chars) to be displayed on GitHub. Please check this report on GCS.

  1. https://storage.googleapis.com/firebase-sdk-metric-reports/fhcPcfeRQG/index.html

*
* @see [Change.toReleaseNote]
*/
private val NON_LINK_BRACKETS_REGEX = Regex("\\[(.+?)](?!\\()", RegexOption.MULTILINE)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This regex will match [foo](bar.baz) lorem ipsum [qud] and could be adjusted to not match ] characters outside of the terminating one.

@rlazo
Copy link
Collaborator

rlazo commented Feb 24, 2024

In (most)? cases, what we want is the reference to be transformed from [app_check] to {{app_check}}, see https://github.com/firebase/firebase-android-sdk/blob/master/appcheck/firebase-appcheck-debug/CHANGELOG.md and cl/605317607 rather than removed

Copy link
Contributor

Unit Test Results

   862 files   -   33     862 suites   - 33   31m 9s ⏱️ -11s
5 186 tests ±    0  5 165 ✔️ +    1  21 💤 ±  0  0  - 1 
9 229 runs   - 244  9 203 ✔️  - 227  26 💤  - 16  0  - 1 

Results for commit d009d78. ± Comparison against base commit 2865f2b.

@rlazo
Copy link
Collaborator

rlazo commented Feb 28, 2024

@rachelsaunders does tooling break or complains if an unknown value is present inside the double brackets? e.g. {{notaname}}, does it complain/break or silently drops it?

@rachelsaunders
Copy link
Contributor

@rachelsaunders does tooling break or complains if an unknown value is present inside the double brackets? e.g. {{notaname}}, does it complain/break or silently drops it?

If the contents within the double curly braces is not a valid string, our internal staging/build tooling will error (and maybe even break the build). It won't just silently drop it. So, if the string happens to be wrong here in the changelog and thus the conversion into the internal systems, we'll be able to easily catch it downstream before it goes to prod.

@rlazo
Copy link
Collaborator

rlazo commented Feb 28, 2024

@rachelsaunders does tooling break or complains if an unknown value is present inside the double brackets? e.g. {{notaname}}, does it complain/break or silently drops it?

If the contents within the double curly braces is not a valid string, our internal staging/build tooling will error (and maybe even break the build). It won't just silently drop it. So, if the string happens to be wrong here in the changelog and thus the conversion into the internal systems, we'll be able to easily catch it downstream before it goes to prod.

That's what I was hoping for, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants