Deploy purchaserTester: clean up dry-run parameter #4140

aboedo · 2024-07-31T19:46:16Z

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

aboedo · 2024-07-31T19:49:34Z

.circleci/config.yml

+      dry_run:
+        type: boolean
+        default: false
    environment:


this bit I don't love: I had to make it a parameter of base job rather than the concrete deploy purchase tester job.

It doesn't actually affect anything other than the fact that all jobs get this param now. Maybe that's deceiving in that if you run the other deploy stuff with dry_run: true, it's not an actual dry run, and that's a bit concerning.

But I couldn't find a better way of keeping it specific to the job: the thing is that we inherit all the stuff from the base job, and that includes parameters. If I declare it again, it breaks the config.

I could also not inherit from base job and just copy/paste all of its things, but I feel like that's worse than what we had before.

I'm open to other ideas, especially since overall this doesn't entirely feel better to me than having two almost identical jobs. I'm worried that a future dev could think that dry_run on all jobs actually means dry run.

Right, I certainly don't love it either and it can be confusing... Not aware of any other options off the top of my head, so I agree with you we might as well leave it as 2 separate jobs.

I think the base job should actually be an executor not an alias:

executors: macos-executor: parameters: xcode_version: type: string default: '15.3' install_swiftlint: type: boolean default: true macos: xcode: << parameters.xcode_version >> resource_class: macos.m1.medium.gen1 environment: CIRCLECI_TESTS_GENERATE_SNAPSHOTS: << pipeline.parameters.generate_snapshots >> CIRCLECI_TESTS_GENERATE_REVENUECAT_UI_SNAPSHOTS: << pipeline.parameters.generate_revenuecatui_snapshots >> working_directory: ~/purchases-ios shell: /bin/bash --login -o pipefail

We can keep the alias so we don't have to update all jobs now:

aliases: base-job: &base-job executor: name: macos-executor

Then you can have custom parameters for the job

deploy-purchase-tester: executor: name: macos-executor parameters: dry_run: type: boolean default: false

I haven't tested it but the config passes CircleCI validation and it matches CircleCI's documentation

ohh that's great, I'll give it a shot. Thanks!

Updated with this approach, looks soooo much better! I hope that it does work. If it does I'll do another follow-up to remove the old base job alias

I ran into some issues with failing tests when mixing and matching this approach and the alias... And this approach is just better than the alias, so I went ahead and replaced it altogether

… can use the dry run parameter directly without a duplicate job

aboedo · 2024-08-06T20:38:21Z

@RCGitBot please test

aboedo · 2024-08-06T21:22:31Z

.circleci/config.yml

-aliases:
-  base-job: &base-job
-    resource_class: macos.m1.medium.gen1
-    macos:
-      xcode: << parameters.xcode_version >>
+executors:


entirely replaces the base job alias with the executor

Much better!

aboedo · 2024-08-06T21:23:28Z

.circleci/config.yml

-      install_swiftlint:
-        type: boolean
-        default: true


This was honestly a pretty deceiving one - you would have to re-define it as a param, but then also remember to pass it in, which was kinda weird (you'll see what I mean in other comments).
And it's not super useful, so I removed it so we can use it at the job level instead

aboedo · 2024-08-06T21:23:39Z

.circleci/config.yml

+    executor:
+      name: macos-executor
+      xcode_version: '14.3.0'


regular usage is just so much cleaner

Yea, I think it's not a bad idea to use aliases only sparingly, because of the inflexibility they often introduce.

aboedo · 2024-08-06T21:24:27Z

.circleci/config.yml

-      install_swiftlint:
-        type: boolean
-        default: false


these jobs didn't actually install swiftlint, even if you pass in true here, since that happens if you call install dependencies, so you could have thought that it should work and gone crazy trying to figure it out

aboedo · 2024-08-06T21:24:48Z

.circleci/config.yml

-    <<: *base-job
-    parameters:
-      xcode_version:
-        type: string
-        default: '14.3.0'
-      install_swiftlint:
-        type: boolean
-        default: false
+    executor:
+      name: macos-executor
+      xcode_version: '14.3.0'


again, this just looks nicer, and you don't have to redefine the parameter every time including the type

aboedo · 2024-08-06T21:25:06Z

.circleci/config.yml

-          install_swiftlint: << parameters.install_swiftlint >>
+          install_swiftlint: false


here I just use the param directly, I feel like this is just much clearer and less error-prone since you don't have to pass in the param

aboedo · 2024-08-06T21:28:15Z

.circleci/config.yml

@@ -1233,7 +1193,7 @@ jobs:
      - update-spm-installation-commit
      - run:
          name: Submit Purchase Tester
-          command: bundle exec fastlane deploy_purchase_tester dry_run:true
+          command: bundle exec fastlane deploy_purchase_tester dry_run:<< parameters.dry_run >>


and this is the actual meat of the PR: being able to define a parameter at the job level easily and then use it

aboedo · 2024-08-06T21:28:50Z

.circleci/config.yml

-      - deploy-purchase-tester-dry-run
      - api-tests
+      - deploy-purchase-tester:
+          dry_run: true


added the param and then moved it to the bottom so it looked a liiiitle nicer

aboedo · 2024-08-06T21:29:28Z

@RCGitBot please test

aboedo · 2024-08-06T21:32:22Z

oops in an earlier commit I didn't pass in the dry run param for deploy purchase tester so I did send out a purchase tester build to TF unintended 😅 Not exactly a problem since it's just an internal testing app

aboedo · 2024-08-06T23:06:39Z

@RevenueCat/coresdk this is ready for another pass

tonidero

Very nice!!

JayShortway

Nice improvement!

@tonidero

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

@aboedo

**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)

@tonidero

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

@aboedo

**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)

aboedo added the ci label Jul 31, 2024

aboedo self-assigned this Jul 31, 2024

aboedo commented Jul 31, 2024

View reviewed changes

aboedo requested review from vegaro and tonidero July 31, 2024 19:49

update base job to make it an executor instead of an alias so that we…

408b5e8

… can use the dry run parameter directly without a duplicate job

aboedo force-pushed the andy/dry-run-parameter-cleanup-deploy-purchasetester branch from 4ede01c to 408b5e8 Compare August 6, 2024 20:37

aboedo added 8 commits August 6, 2024 18:04

replaced the base job with executor in a few places

ead62e2

checkpoint after updating a few more things

690c5e2

another checkpoint removing the install_swiftlint param

f595ba1

one more checkpoint

0cb708d

and one more

165e82c

final pass at replacing base job

c83cb2d

removed the base job alias

1c3d27c

re-add swiftlint skip to one job

3e14dbe

aboedo commented Aug 6, 2024

View reviewed changes

tonidero approved these changes Aug 7, 2024

View reviewed changes

JayShortway approved these changes Aug 7, 2024

View reviewed changes

aboedo merged commit af94fb1 into main Aug 7, 2024
33 checks passed

aboedo deleted the andy/dry-run-parameter-cleanup-deploy-purchasetester branch August 7, 2024 13:48

RCGitBot mentioned this pull request Aug 7, 2024

[AUTOMATIC] Release/5.2.3 #4153

Closed

RCGitBot mentioned this pull request Aug 7, 2024

[AUTOMATIC] Release/5.2.3 #4155

Merged

vegaro added the pr:other label Sep 17, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deploy purchaserTester: clean up dry-run parameter #4140

Deploy purchaserTester: clean up dry-run parameter #4140

aboedo commented Jul 31, 2024

aboedo Jul 31, 2024

tonidero Aug 1, 2024

vegaro Aug 1, 2024 •

edited

Loading

aboedo Aug 5, 2024

aboedo Aug 6, 2024

aboedo Aug 6, 2024

aboedo commented Aug 6, 2024

aboedo Aug 6, 2024

JayShortway Aug 7, 2024

aboedo Aug 6, 2024

aboedo Aug 6, 2024

JayShortway Aug 7, 2024

aboedo Aug 6, 2024

aboedo Aug 6, 2024

aboedo Aug 6, 2024

aboedo Aug 6, 2024

aboedo Aug 6, 2024

aboedo commented Aug 6, 2024

aboedo commented Aug 6, 2024 •

edited

Loading

aboedo commented Aug 6, 2024

tonidero left a comment

JayShortway left a comment

		install_swiftlint: << parameters.install_swiftlint >>
		install_swiftlint: false

Deploy purchaserTester: clean up dry-run parameter #4140

Deploy purchaserTester: clean up dry-run parameter #4140

Conversation

aboedo commented Jul 31, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vegaro Aug 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aboedo commented Aug 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aboedo commented Aug 6, 2024

aboedo commented Aug 6, 2024 • edited Loading

aboedo commented Aug 6, 2024

tonidero left a comment

Choose a reason for hiding this comment

JayShortway left a comment

Choose a reason for hiding this comment

vegaro Aug 1, 2024 •

edited

Loading

aboedo commented Aug 6, 2024 •

edited

Loading