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

RUMM-1744 Update release tool to conditionally validate Kronos.xcframework #704

Conversation

ncreated
Copy link
Member

@ncreated ncreated commented Jan 3, 2022

What and why?

📦 This PR updates our release automation so it can conditionally include/exclude certain XCFrameworks from GitHub release asset validation (.*zip). This is required as starting from 1.9.0 SDK will no longer depend on Kronos.xcframework (#701), but when (re)building historical tags, we want it to be included.

How?

I extracted the code for validating each XCFramework and used OCP to make it configurable for a certain range of Version.

Review checklist

  • Feature or bugfix MUST have appropriate tests (unit, integration)
  • Make sure each commit and the PR mention the Issue number or JIRA reference

@ncreated ncreated self-assigned this Jan 3, 2022
@ncreated ncreated requested a review from a team as a code owner January 3, 2022 15:08
@ncreated ncreated force-pushed the ncreated/RUMM-1744-update-release-tool-to-conditionally-include-Kronos branch from ce488b6 to 0d517e5 Compare January 3, 2022 15:38
Copy link
Contributor

@buranmert buranmert left a comment

Choose a reason for hiding this comment

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

i'm not sure if this is the simplest solution but as long as it works it's fine for me

Copy link
Member

@maxep maxep left a comment

Choose a reason for hiding this comment

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

LGTM!

@ncreated
Copy link
Member Author

ncreated commented Jan 4, 2022

i'm not sure if this is the simplest solution but as long as it works it's fine for me

@buranmert do you see anything in particular that we could simplify? My goal was to have separate validation for each XCFramework and this way we can assert more critical aspects: swift compatibility, architecture slices and dSYMs presence. Given that soon we will produce more slices (V2, next platforms), I think this approach should be reliable and easy to scale.

@buranmert
Copy link
Contributor

it's just seeing +468 −37 made me wonder if that was the simplest solution.
i mean as far as i can see we do basically

if version < SOME_VERSION
  kronos_should_not_exist()
else
  kronos_should_exist_with_its_content()

@ncreated
Copy link
Member Author

ncreated commented Jan 4, 2022

it's just seeing +468 −37 made me wonder if that was the simplest solution. i mean as far as i can see we do basically

if version < SOME_VERSION
  kronos_should_not_exist()
else
  kronos_should_exist_with_its_content()

It would need another if/else for crash reporting (1.7.0+) and several more for next platforms (tvOS / watchOS) + eventual modules in V2, ending up with if/else pyramid. Given no integration tests for this tool, introducing complex and not tested control flows sounds error prone which is where basic and declarative OCP should help.

@ncreated ncreated merged commit 3f47ccc into ncreated/RUMM-1744-embed-Kronos-directly-into-SDK Jan 5, 2022
@ncreated ncreated deleted the ncreated/RUMM-1744-update-release-tool-to-conditionally-include-Kronos branch January 5, 2022 09:41
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