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

[InAppMessaging] Consolidate FIAM and FIAMSwift #11800

Merged
merged 27 commits into from
Oct 13, 2023
Merged

Conversation

ncooke3
Copy link
Member

@ncooke3 ncooke3 commented Sep 12, 2023

Context

  • The contents of FIAM's Swift extension SDK has been moved into the main FIAM module, and the FIAM Swift extension SDK now re-exports the API that used to live in it.
  • This change should be non-breaking.

Tasks

  • The affected extension podspecs should be versioned to pin to the releasing version.
  • When this PR is merged, stage the affected podspecs and notify the Games team.

Important

@google-oss-bot google-oss-bot added the api: inappmessaging Firebase In App Messaging label Sep 12, 2023
@google-oss-bot
Copy link

google-oss-bot commented Sep 12, 2023

Coverage Report 1

Affected Products

  • FirebaseInAppMessaging-iOS-FirebaseInAppMessaging.framework

    Overall coverage changed from 46.22% (1e0f294) to 44.72% (3935dea) by -1.50%.

    FilenameBase (1e0f294)Merge (3935dea)Diff
    CustomInAppMessageDisplayViewModifier.swift?0.00%?
    SwiftUIPreviewHelpers.swift?0.00%?

Test Logs

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

@ncooke3 ncooke3 force-pushed the fiam-swift-merged branch 3 times, most recently from b5dcd63 to b31c3fb Compare September 13, 2023 16:28
@google-oss-bot
Copy link

Size Report 1

Affected Products

  • FirebaseInAppMessaging

    TypeBase (0c9fe27)Merge (4e1a0f3)Diff
    CocoaPods?-51.5 kB? (?)

Test Logs

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

@ncooke3 ncooke3 closed this Sep 14, 2023
@ncooke3 ncooke3 reopened this Sep 14, 2023
@ncooke3
Copy link
Member Author

ncooke3 commented Sep 14, 2023

Closed and re-opened to deflate some transient network failures in CI.

@ncooke3
Copy link
Member Author

ncooke3 commented Sep 16, 2023

I'm a bit unsure of how to fix spectesting / specs_testing (FirebaseInAppMessagingSwift.podspec) (pull_request).

After reproducing locally, it seems that the FirebaseInAppMessagingSwift.podspec's dependency on FirebaseInAppMessaging.podspec isn't coming from the FirebaseInAppMessagingSwift.podspec that has been modified in this PR! Therefore, FirebaseInAppMessagingSwift.podspec doesn't lint successfully because it really needs the updated FirebaseInAppMessaging.podspec that is also changed in this PR.

I'm able to get things linting successfully if I lint the specs together:

pod spec lint FirebaseInAppMessagingSwift.podspec FirebaseInAppMessaging.podspec --platforms=ios --skip-tests --allow-warnings --sources=https://github.com/firebase/SpecsTesting,https://cdn.cocoapods.org/

I'd imagine this should workflow failure may happen anytime multiple specs are updated in a single PR where the changes are heavily dependent.

Is this considered a bug in the workflow? Should the workflow lint all changed specs together rather than breaking them up into a job matrix where each spec is linted separately?

@paulb777
Copy link
Member

It's a known issue we've seen before. Historically, we've ignored interdependent spectesting issues like this until after merging and updating tags.

Let's discuss next week

@ncooke3 ncooke3 force-pushed the fiam-swift-merged branch 3 times, most recently from 9854c6a to 7d662ff Compare October 12, 2023 13:38
@ncooke3 ncooke3 marked this pull request as ready for review October 12, 2023 13:38
FirebaseInAppMessaging.podspec Outdated Show resolved Hide resolved
Copy link
Member

@paulb777 paulb777 left a comment

Choose a reason for hiding this comment

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

May also want to notify @mikehardy when the consolidation podspecs are staged.

@ncooke3 ncooke3 merged commit b962ad6 into master Oct 13, 2023
64 of 65 checks passed
@ncooke3 ncooke3 deleted the fiam-swift-merged branch October 13, 2023 18:52
@firebase firebase locked and limited conversation to collaborators Nov 13, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api: inappmessaging Firebase In App Messaging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants