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

[CHNL-6170] updated EventName cases in KlaviyoBridge #172

Merged
merged 10 commits into from
Sep 24, 2024

Conversation

ab1470
Copy link
Contributor

@ab1470 ab1470 commented Sep 10, 2024

Description

This updates the KlaviyoBridge class to account for the changes made to EventName as part of Swift SDK #196

Check List

  • Are you changing anything with the public API?
  • Are your changes backwards compatible with previous SDK Versions?

ab1470 and others added 2 commits September 10, 2024 14:37
* Adopting ios modularization stuff

* added a comment to the podfile
@wiz-inc-faae60d47d
Copy link

wiz-inc-faae60d47d bot commented Sep 13, 2024

Wiz Scan Summary

Scan Module Critical High Medium Low Info Total
IaC Misconfigurations 0 0 0 0 0 0
Vulnerabilities 2 3 0 0 0 5
Secrets 0 0 0 0 0 0
Total 2 3 0 0 0 5

View scan details in Wiz

To detect these findings earlier in the dev lifecycle, try using Wiz Code VS Code Extension.

@ab1470 ab1470 marked this pull request as ready for review September 20, 2024 18:27
@ab1470 ab1470 requested a review from a team as a code owner September 20, 2024 18:27
@ab1470 ab1470 requested review from jordan-griffin and removed request for a team September 20, 2024 18:27
Copy link

@ndurell ndurell left a comment

Choose a reason for hiding this comment

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

Looks pretty harmless!

:app_path => "#{Pod::Config.instance.installation_root}/.."
)

#TODO: remove this once iOS modularization release is made and the podspec is updated
pod 'KlaviyoSwift', :path => '../../../klaviyo-swift-sdk/'
Copy link

Choose a reason for hiding this comment

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

I think build is failing because of this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ajaysubra @ndurell are we concerned about the failing build at the moment? This change is only temporary so we can work locally until the Swift SDK v4.0.0 is released. Then we revert the changes and CI should build again.

If we want to get CI to pass we can point to the remote git branch rather than the relative path. But then when we're working on it locally we'll probably want to change it back to the relative path

Copy link

Choose a reason for hiding this comment

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

Uhh do we have to merge with this in? I'd prefer to keep master in a non broken state if possible.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah you are right. Removed it 🤞🏾

Copy link
Collaborator

Choose a reason for hiding this comment

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

thanks for checking @ndurell and @ab1470

Copy link
Collaborator

Choose a reason for hiding this comment

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

So the build is still failing. Went on a wild goose chase and found out that even with this line removed the build will fail because the Event name changes in here are not in the release swift version that the podspec is pointing to. Unfortunately, the podspec can't point to a branch of the swift SDK.

2024-09-24T02:50:43.2997240Z ** BUILD FAILED **
2024-09-24T02:50:43.2997360Z 
2024-09-24T02:50:43.2997370Z 
2024-09-24T02:50:43.2997490Z The following build commands failed:
2024-09-24T02:50:43.2998390Z 	SwiftCompile normal x86_64 /Users/runner/work/klaviyo-react-native-sdk/klaviyo-react-native-sdk/ios/KlaviyoBridge.swift (in target 'klaviyo-react-native-sdk' from project 'Pods')
2024-09-24T02:50:43.2999890Z 	SwiftCompile normal x86_64 Compiling\ KlaviyoBridge.swift /Users/runner/work/klaviyo-react-native-sdk/klaviyo-react-native-sdk/ios/KlaviyoBridge.swift (in target 'klaviyo-react-native-sdk' from project 'Pods')
2024-09-24T02:50:43.3000780Z (2 failures)
2024-09-24T02:50:43.3001900Z [ERROR] command finished with error: command (/Users/runner/work/klaviyo-react-native-sdk/klaviyo-react-native-sdk/example) /private/var/folders/4h/j1rdr7ws1cbck70c6bjbk5t40000gn/T/xfs-f23266fe/yarn run build:ios exited (65)

I suggest we keep this line in and merge mainly because we could at least work on this locally and when we release iOS we can get this build passing again. Long term with a WIP swift/android branch we need to figure out how to work.

@ab1470 ab1470 merged commit 42e8933 into master Sep 24, 2024
8 of 9 checks passed
@ab1470 ab1470 deleted the ab/CHNL-6170/update-EventName-cases-in-RN-bridge branch September 24, 2024 21:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants