Skip to content

More project structure cleanup#4131

Merged
aboedo merged 13 commits into
mainfrom
andy/more_structure_cleanup
Jul 31, 2024
Merged

More project structure cleanup#4131
aboedo merged 13 commits into
mainfrom
andy/more_structure_cleanup

Conversation

@aboedo

@aboedo aboedo commented Jul 29, 2024

Copy link
Copy Markdown
Member

More project structure cleanup:

  • Removes redundant workspace and project files for PurchaseTester
  • Cleans up bad references
  • Cleans up certificates
  • Re-does the process to deploy PurchaseTester, since it was designed to run from the xcworkspace, which doesn't work anymore.
  • adds a dry-run option to deploying PurchaseTester and adds a job for it in the all-tests workflow. It tries building it and archiving but doesn't actually publish to TestFlight. We didn't really have a test for this, so it's another thing that could have broken in production.

@aboedo aboedo added the ci label Jul 29, 2024
@aboedo aboedo self-assigned this Jul 29, 2024
@aboedo

aboedo commented Jul 29, 2024

Copy link
Copy Markdown
Member Author

@RCGitBot please test

@aboedo

aboedo commented Jul 30, 2024

Copy link
Copy Markdown
Member Author

@RCGitBot please test

@aboedo aboedo force-pushed the andy/more_structure_cleanup branch from bb60bf8 to 4cedaa6 Compare July 30, 2024 18:46
@aboedo

aboedo commented Jul 30, 2024

Copy link
Copy Markdown
Member Author

@RCGitBot please test

@aboedo aboedo marked this pull request as ready for review July 30, 2024 19:20
Comment thread .circleci/config.yml
- update-spm-installation-commit
- run:
name: Submit Purchase Tester
working_directory: "Tests/TestingApps/PurchaseTesterSwiftUI"

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

no longer needed or desired, we want the jobs to run from the root since that's where the framework is generated

Comment thread .circleci/config.yml
Comment on lines -1328 to +1342
# temporarily disabled since this won't work when building directly from its workspace.
# there's another PR to re-enable it
# - deploy-purchase-tester:
# requires:
# - make-release
# <<: *release-tags
- deploy-purchase-tester:
requires:
- make-release
<<: *release-tags

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

we can re-enable this now

Comment thread .circleci/config.yml
- installation-tests-carthage
- installation-tests-xcode-direct-integration
- installation-tests-receipt-parser
- deploy-purchase-tester-dry-run

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

dry run job in all-tests so that we know if we're breaking purchase tester (I didn't realize that I had broken it until Toni mentioned it in a PR comment)

2DD500E22C519EB4009C19B7 /* PurchaseTesterApp.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = PurchaseTesterApp.swift; sourceTree = "<group>"; };
2DD500E32C519EB4009C19B7 /* ReceiptInspector.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = ReceiptInspector.swift; sourceTree = "<group>"; };
2DD500E42C519EB4009C19B7 /* ReceiptVerifier.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = ReceiptVerifier.swift; sourceTree = "<group>"; };
2DD500E52C519EB4009C19B7 /* RevenueCat_SwiftUIConfiguration.storekit */ = {isa = PBXFileReference; lastKnownFileType = text; path = RevenueCat_SwiftUIConfiguration.storekit; sourceTree = "<group>"; };

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

cleanup, this no longer exists

2DD500EA2C519EB4009C19B7 /* PurchaseTester-Info.plist */ = {isa = PBXFileReference; lastKnownFileType = text.plist.xml; path = "PurchaseTester-Info.plist"; sourceTree = "<group>"; };
2DD500EB2C519EB4009C19B7 /* PurchaseTester.entitlements */ = {isa = PBXFileReference; lastKnownFileType = text.plist.entitlements; path = PurchaseTester.entitlements; sourceTree = "<group>"; };
2DD500EC2C519EB4009C19B7 /* PurchaseTester.xcodeproj */ = {isa = PBXFileReference; lastKnownFileType = "wrapper.pb-project"; path = PurchaseTester.xcodeproj; sourceTree = "<group>"; };
2DD500ED2C519EB4009C19B7 /* PurchaseTester.xcworkspace */ = {isa = PBXFileReference; lastKnownFileType = wrapper.workspace; path = PurchaseTester.xcworkspace; sourceTree = "<group>"; };

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

removed the workspace since it doesn't add any more value

Comment thread fastlane/Fastfile
'./Tests/InstallationTests/ReceiptParserInstallation/ReceiptParserInstallation.xcodeproj/project.pbxproj',
'./Tests/APITesters/CustomEntitlementComputationSwiftAPITester/CustomEntitlementComputationSwiftAPITester.xcodeproj/project.pbxproj',
'./Tests/APITesters/RevenueCatUIAPITester/RevenueCatUISwiftAPITester.xcodeproj/project.pbxproj',
'./Tests/TestingApps/PurchaseTesterSwiftUI/PurchaseTester.xcodeproj/project.pbxproj',

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I had missed this but we no longer have a hardcoded commit in that file since we're now using a local dependency instead, so this is unnecessary

Comment thread fastlane/Fastfile
Comment on lines -869 to -870
lane :deploy_purchase_tester do
latest_changelog = File.read("#{File.dirname(__FILE__)}/../#{changelog_latest_path}")

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

quite a few changes here, but they ultimately boil down to updating things so that we're running the lane from the repo root rather than PurchaseTester root.
This is because we do need to get the RevenueCat SDK that's generated at the top level. It wasn't needed before since we were using an SPM dependency on a hardcoded commit instead, but the local dependency is much easier to work with on a daily basis.

Comment thread fastlane/Fastfile
Comment on lines -910 to -911
ios_build(changelog)
macos_build(changelog)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I also removed the local functions because I wanted to reuse a couple of the parameters without having to pass them in over and over. I can re-add them, but I don't usually find local functions to be easier to read. Thoughts?

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.

I'm ok with that. I think it only could make some sense in some situations where the code needs to be executed multiple times, so I agree with the change 👍

Comment thread fastlane/Fastfile
Comment on lines +897 to +900
provisioningProfiles: {
"com.revenuecat.sampleapp" => "match AppStore com.revenuecat.sampleapp",
"com.revenuecat.sampleapp.watchkitapp" => "match AppStore com.revenuecat.sampleapp.watchkitapp"
}

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I ended up passing the profiles explicitly here. I'm not entirely sure what I'm missing, but it seems like fastlane was reading the values from the Matchfile previously. I don't think there's a way for fastlane to read the Matchfile from a different directory.

Maybe @joshdholtz knows? Maybe I could just import it?

Comment thread scripts/pre-commit.sh
"${SCRIPT_DIR}/../Examples/MagicWeather/MagicWeather/Constants.swift"
"${SCRIPT_DIR}/../Examples/MagicWeatherSwiftUI/Shared/Constants.swift"
"${SCRIPT_DIR}/../Tests/TestingApps/PurchaseTesterSwiftUI/Core/Constants.swift"
"${SCRIPT_DIR}/../Tests/TestingApps/PurchaseTester/PurchaseTester/Constants.swift"

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This was for the old purchaseTester UIKit, no longer exists so no longer needed

@aboedo aboedo requested a review from a team July 30, 2024 19:28

@tonidero tonidero left a comment

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.

I tested it out for a bit and looks good to me!

Comment thread .circleci/config.yml
- update-spm-installation-commit
- run:
name: Submit Purchase Tester
command: bundle exec fastlane deploy_purchase_tester dry_run:true

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.

Hmm I guess the dry_run could be a parameter of the job... But maybe this is better so it's more delimited, I'm ok keeping it as is.

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.

I think it should be a parameter, main reason is that if we update the deploy-purchase-tester job, we will probably forget about updating this other job. Having a single job with a parameter will make sure we only have to update one thing

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

That's a great point. It's slightly tricky because of how we re-use jobs, but it's still very easy. I'll open up a separate PR for it since this one is approved and so that we can have a specific conversation about it

Comment thread fastlane/Fastfile
Comment on lines -910 to -911
ios_build(changelog)
macos_build(changelog)

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.

I'm ok with that. I think it only could make some sense in some situations where the code needs to be executed multiple times, so I agree with the change 👍

@aboedo aboedo merged commit 28b5f2d into main Jul 31, 2024
@aboedo aboedo deleted the andy/more_structure_cleanup branch July 31, 2024 19:43
aboedo added a commit that referenced this pull request Aug 7, 2024
Cleans up the deploy purchase tester job's treatment of `dry-run`: it's
now a parameter of the job instead of a separate job entirely.

This is based off of @tonidero and @vegaro's feedback in #4131
jamesrb1 pushed a commit that referenced this pull request Aug 7, 2024
## More project structure cleanup: 

- Removes redundant workspace and project files for PurchaseTester
- Cleans up bad references
- Cleans up certificates
- Re-does the process to deploy PurchaseTester, since it was designed to
run from the xcworkspace, which doesn't work anymore.
- adds a `dry-run` option to deploying PurchaseTester and adds a job for
it in the all-tests workflow. It tries building it and archiving but
doesn't actually publish to TestFlight. We didn't really have a test for
this, so it's another thing that could have broken in production.
jamesrb1 pushed a commit that referenced this pull request Aug 7, 2024
Cleans up the deploy purchase tester job's treatment of `dry-run`: it's
now a parameter of the job instead of a separate job entirely.

This is based off of @tonidero and @vegaro's feedback in #4131
MarkVillacampa pushed a commit that referenced this pull request Aug 7, 2024
**This is an automatic release.**

### Bugfixes
* Fix Paywalls crash on iOS 18 beta (#4154) via Andy Boedo (@aboedo)
### Dependency Updates
* Bump danger from 9.4.3 to 9.5.0 (#4143) via dependabot[bot]
(@dependabot[bot])
* Bump nokogiri from 1.16.6 to 1.16.7 (#4129) via dependabot[bot]
(@dependabot[bot])
* Bump fastlane from 2.221.1 to 2.222.0 (#4130) via dependabot[bot]
(@dependabot[bot])
### Other Changes
* Update deployment targets for tests (#4145) via Andy Boedo (@aboedo)
* Deploy purchaserTester: clean up dry-run parameter (#4140) via Andy
Boedo (@aboedo)
* Clean up API Testers (#4141) via Andy Boedo (@aboedo)
* More project structure cleanup (#4131) via Andy Boedo (@aboedo)
* temporarily disables purchasetester deploy (#4133) via Andy Boedo
(@aboedo)
* Fix trigger all tests branch (#4135) via Andy Boedo (@aboedo)
* Clean up XCWorkspace and testing apps (#4111) via Andy Boedo (@aboedo)
* tests trigger: add target-branch parameter to trigger from the right
branch (#4121) via Andy Boedo (@aboedo)
* Re-added the RevenueCatUI tests job on every commit (#4113) via Andy
Boedo (@aboedo)
nyeu pushed a commit that referenced this pull request Oct 2, 2024
## More project structure cleanup: 

- Removes redundant workspace and project files for PurchaseTester
- Cleans up bad references
- Cleans up certificates
- Re-does the process to deploy PurchaseTester, since it was designed to
run from the xcworkspace, which doesn't work anymore.
- adds a `dry-run` option to deploying PurchaseTester and adds a job for
it in the all-tests workflow. It tries building it and archiving but
doesn't actually publish to TestFlight. We didn't really have a test for
this, so it's another thing that could have broken in production.
nyeu pushed a commit that referenced this pull request Oct 2, 2024
Cleans up the deploy purchase tester job's treatment of `dry-run`: it's
now a parameter of the job instead of a separate job entirely.

This is based off of @tonidero and @vegaro's feedback in #4131
nyeu pushed a commit that referenced this pull request Oct 2, 2024
**This is an automatic release.**

### Bugfixes
* Fix Paywalls crash on iOS 18 beta (#4154) via Andy Boedo (@aboedo)
### Dependency Updates
* Bump danger from 9.4.3 to 9.5.0 (#4143) via dependabot[bot]
(@dependabot[bot])
* Bump nokogiri from 1.16.6 to 1.16.7 (#4129) via dependabot[bot]
(@dependabot[bot])
* Bump fastlane from 2.221.1 to 2.222.0 (#4130) via dependabot[bot]
(@dependabot[bot])
### Other Changes
* Update deployment targets for tests (#4145) via Andy Boedo (@aboedo)
* Deploy purchaserTester: clean up dry-run parameter (#4140) via Andy
Boedo (@aboedo)
* Clean up API Testers (#4141) via Andy Boedo (@aboedo)
* More project structure cleanup (#4131) via Andy Boedo (@aboedo)
* temporarily disables purchasetester deploy (#4133) via Andy Boedo
(@aboedo)
* Fix trigger all tests branch (#4135) via Andy Boedo (@aboedo)
* Clean up XCWorkspace and testing apps (#4111) via Andy Boedo (@aboedo)
* tests trigger: add target-branch parameter to trigger from the right
branch (#4121) via Andy Boedo (@aboedo)
* Re-added the RevenueCatUI tests job on every commit (#4113) via Andy
Boedo (@aboedo)
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.

3 participants