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

More project structure cleanup #4131

Merged
merged 13 commits into from
Jul 31, 2024
Merged

More project structure cleanup #4131

merged 13 commits into from
Jul 31, 2024

Conversation

aboedo
Copy link
Member

@aboedo aboedo commented Jul 29, 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.

@aboedo aboedo added the ci label Jul 29, 2024
@aboedo aboedo self-assigned this Jul 29, 2024
@aboedo
Copy link
Member Author

aboedo commented Jul 29, 2024

@RCGitBot please test

@aboedo
Copy link
Member Author

aboedo commented Jul 30, 2024

@RCGitBot please test

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

aboedo commented Jul 30, 2024

@RCGitBot please test

@aboedo aboedo marked this pull request as ready for review July 30, 2024 19:20
@@ -1231,9 +1231,20 @@ jobs:
- update-spm-installation-commit
- run:
name: Submit Purchase Tester
working_directory: "Tests/TestingApps/PurchaseTesterSwiftUI"
Copy link
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 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
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

@@ -1421,3 +1430,4 @@ workflows:
- installation-tests-carthage
- installation-tests-xcode-direct-integration
- installation-tests-receipt-parser
- deploy-purchase-tester-dry-run
Copy link
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)

@@ -1200,14 +1200,12 @@
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
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

2DD500E62C519EB4009C19B7 /* Windows.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = Windows.swift; sourceTree = "<group>"; };
2DD500E82C519EB4009C19B7 /* AppIcon.xcassets */ = {isa = PBXFileReference; lastKnownFileType = folder.assetcatalog; path = AppIcon.xcassets; sourceTree = "<group>"; };
2DD500E92C519EB4009C19B7 /* PrivacyInfo.xcprivacy */ = {isa = PBXFileReference; lastKnownFileType = text.xml; path = PrivacyInfo.xcprivacy; sourceTree = "<group>"; };
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
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

@@ -701,7 +701,6 @@ platform :ios do
'./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
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 on lines -869 to -870
lane :deploy_purchase_tester do
latest_changelog = File.read("#{File.dirname(__FILE__)}/../#{changelog_latest_path}")
Copy link
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 on lines -910 to -911
ios_build(changelog)
macos_build(changelog)
Copy link
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
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 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
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?

@@ -38,7 +38,6 @@ verify_no_included_apikeys() {
"${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
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
Copy link
Contributor

@tonidero tonidero left a comment

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!

- update-spm-installation-commit
- run:
name: Submit Purchase Tester
command: bundle exec fastlane deploy_purchase_tester dry_run:true
Copy link
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
Contributor

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
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 on lines -910 to -911
ios_build(changelog)
macos_build(changelog)
Copy link
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
33 checks passed
@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