Skip to content

feat(launchpad): Update artifact factory logic to cover more cases#140

Closed
NicoHinderling wants to merge 2 commits intomainfrom
07-08-feat_launchpad_update_artifact_factory_logic_to_cover_more_cases
Closed

feat(launchpad): Update artifact factory logic to cover more cases#140
NicoHinderling wants to merge 2 commits intomainfrom
07-08-feat_launchpad_update_artifact_factory_logic_to_cover_more_cases

Conversation

@NicoHinderling
Copy link
Copy Markdown
Contributor

@NicoHinderling NicoHinderling commented Jul 8, 2025

When testing on a variety of different xcarchives, I found the existing logic failing to properly identify that it was an xcarchive. These changes fixed all the cases I found

Copy link
Copy Markdown
Contributor Author

NicoHinderling commented Jul 8, 2025

@codecov
Copy link
Copy Markdown

codecov bot commented Jul 8, 2025

Codecov Report

Attention: Patch coverage is 87.14286% with 9 lines in your changes missing coverage. Please review.

Project coverage is 71.78%. Comparing base (d4acf88) to head (1afa006).

Files with missing lines Patch % Lines
src/launchpad/artifacts/apple/zipped_xcarchive.py 64.28% 1 Missing and 4 partials ⚠️
src/launchpad/artifacts/artifact_factory.py 85.18% 2 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #140      +/-   ##
==========================================
+ Coverage   71.63%   71.78%   +0.14%     
==========================================
  Files          93       93              
  Lines        7320     7365      +45     
  Branches      860      863       +3     
==========================================
+ Hits         5244     5287      +43     
  Misses       1724     1724              
- Partials      352      354       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

# Format 1: MyApp.xcarchive/Products/**/*.app
"*.xcarchive/Products/**/*.app",
# Format 2: Products/Applications/*.app (direct extraction)
"Products/Applications/*.app",
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is this something we want to support? I'm a bit confused with these two cases, why wouldn't there be a top-level .xcarchive wrapping these folders?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

+1 I don't think we should support this, or the one below it. We did in emerge, but now we are normalizing all the artifacts (in the CLI) and we should expect everything to have a well defined structure by this point. This will help us reduce a lot of the bugs we saw with bespoke folder structures in emerge

app_bundle_path = app_bundle_path.relative_to(xcarchive_dir)
file_path = xcarchive_dir / "ParsedAssets" / app_bundle_path / json_name
else:
# Format 2: Products/Applications/... (direct extraction)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Same as above, I don't think we should support this case

@NicoHinderling
Copy link
Copy Markdown
Contributor Author

getsentry/sentry-cli#2585

ended up fixing this bug in the CLI instead

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