[HOLD for payment 2023-01-13] [$2000] Create automated check to catch uncommitted changes to `Podfile.lock`

[amyevans]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

Problem

When a developer adds, removes, or updates a package in a PR, there will always be a diff in package.json and package-lock.json, and the changes in both must be committed. Depending on the package, there may or may not be a diff in ios/Podfile.lock as well. Sometimes developers forget to run pod install when modifying a package, so the potential diff in ios/Podfile.lock is not caught until after the PR is merged and a different developer runs pod install. This can lead to confusion.

We have an existing GitHub Action workflow verifyPodfile.yml that uses the script (verifyPodfile.sh)[https://github.com/Expensify/App/blob/main/.github/scripts/verifyPodfile.sh] to verify that Podfile.lock is in sync with Podfile (by getting the checksum for Podfile and ensuring it matches what is printed in Podfile.lock), but that does nothing to catch the case as outlined above.

Solution

Add some sort of automated check to prevent the merging of a PR that causes Podfile.lock to be modified but does not have the diff committed.

This could be a modification to the verifyPodfile script, an entirely new CI check, or anything else really that accomplishes the goal.

Requirements

• Avoid solutions that would require workflow changes for developers, e.g. forcing everyone onto the same cocoapods version, or having to run bundle exec pod install instead of pod install. We have a large developer community which makes it challenging to communicate and enforce process changes like these.
• It should be automated (e.g. not something vulnerable to human error, like simply adding another checkbox to our PR checklist)
  Upwork Automation - Do Not Edit

  • Upwork Job URL: https://www.upwork.com/jobs/~01baa80d227834e4fb
  • Upwork Job ID: 1600941999446802432
  • Last Price Increase: 2022-12-19

[melvin-bot[bot]]

Triggered auto assignment to @JmillsExpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

[amyevans]

Locking temporarily to get internal feedback around the framing of the issue, and then I'll open it up for external help.

Internal reference issue: https://github.com/Expensify/Expensify/issues/211674

[AndrewGable]

An alternative solution that I've discussed with @roryabraham is we should lock all package versions in package.json to a specific version, which I think would prevent the root cause of this from happening. Then we could add some lint rule to check for locked version in package.json

Thoughts?

[amyevans]

I don't think it prevents the root cause because you still need to manually run pod install to get any diff in Podfile.lock.

Also locking down to a specific version in package.json means we'd need to get more intentional about checking for updates to packages and installing them, right? Or do we have Snyk fully integrated and checking for those updates, I don't quite recall?

[AndrewGable]

It's my understanding that when package.json tells pod install what version of pod to install via autolinking, so while yes, you do need to run pod install it's actually the npm install ran before pod install that will change the version in Podfile.lock. Does that make sense?

Currently, I believe we do not update packages unless there is a security vulnerability or a feature we want in the latest version. Synk checks are running to check for those vulnerabilities.

[amyevans]

To ground in an example,

I run npm i react-native-permissions@3.3.1 and that produces the following diff:

diff --git a/package-lock.json b/package-lock.json
index d111a34f48..8e603632ae 100644
--- a/package-lock.json
+++ b/package-lock.json
@@ -71,7 +71,7 @@
         "react-native-onyx": "1.0.29",
         "react-native-pdf": "^6.6.2",
         "react-native-performance": "^2.0.0",
-        "react-native-permissions": "^3.0.1",
+        "react-native-permissions": "^3.3.1",
         "react-native-picker-select": "git+https://github.com/Expensify/react-native-picker-select.git#7f09b2c15ffae320d769788f75bdf8948714bb10",
         "react-native-plaid-link-sdk": "^7.2.0",
         "react-native-reanimated": "3.0.0-rc.6",
@@ -35503,9 +35503,9 @@
       }
     },
     "node_modules/react-native-permissions": {
-      "version": "3.6.1",
-      "resolved": "https://registry.npmjs.org/react-native-permissions/-/react-native-permissions-3.6.1.tgz",
-      "integrity": "sha512-fzPpPQXeD34inUccqtoResSwYubfrwUguP4qrVUUv8+KSMjYSaHGoS5HaIJLZHlN9gO+TvLJZ2L5ZljTsb6qnQ==",
+      "version": "3.3.1",
+      "resolved": "https://registry.npmjs.org/react-native-permissions/-/react-native-permissions-3.3.1.tgz",
+      "integrity": "sha512-Ap9nVWBWJ7lyU6Ye3Qltm2V+Ut6XjDcs+6ZBmK1UUxcCoSSGx/mQg2GbMJLjlnoVtUgVHR3IhyZ0mN9DlMPRFQ==",
       "peerDependencies": {
         "react": ">=16.13.1",
         "react-native": ">=0.63.3",
@@ -69833,9 +69833,9 @@
       "requires": {}
     },
     "react-native-permissions": {
-      "version": "3.6.1",
-      "resolved": "https://registry.npmjs.org/react-native-permissions/-/react-native-permissions-3.6.1.tgz",
-      "integrity": "sha512-fzPpPQXeD34inUccqtoResSwYubfrwUguP4qrVUUv8+KSMjYSaHGoS5HaIJLZHlN9gO+TvLJZ2L5ZljTsb6qnQ==",
+      "version": "3.3.1",
+      "resolved": "https://registry.npmjs.org/react-native-permissions/-/react-native-permissions-3.3.1.tgz",
+      "integrity": "sha512-Ap9nVWBWJ7lyU6Ye3Qltm2V+Ut6XjDcs+6ZBmK1UUxcCoSSGx/mQg2GbMJLjlnoVtUgVHR3IhyZ0mN9DlMPRFQ==",
       "requires": {}
     },
     "react-native-picker-select": {
diff --git a/package.json b/package.json
index 7a2d33b66e..4dcfcfbe94 100644
--- a/package.json
+++ b/package.json
@@ -101,7 +101,7 @@
     "react-native-onyx": "1.0.29",
     "react-native-pdf": "^6.6.2",
     "react-native-performance": "^2.0.0",
-    "react-native-permissions": "^3.0.1",
+    "react-native-permissions": "^3.3.1",
     "react-native-picker-select": "git+https://github.com/Expensify/react-native-picker-select.git#7f09b2c15ffae320d769788f75bdf8948714bb10",
     "react-native-plaid-link-sdk": "^7.2.0",
     "react-native-reanimated": "3.0.0-rc.6",

Once I run cd ios && pod install, I get the additional diff:

diff --git a/ios/Podfile.lock b/ios/Podfile.lock
index c696014a9b..d574d794a4 100644
--- a/ios/Podfile.lock
+++ b/ios/Podfile.lock
@@ -222,13 +222,13 @@ PODS:
     - Onfido (= 27.0.0)
     - React
   - OpenSSL-Universal (1.1.1100)
-  - Permission-Camera (3.6.1):
+  - Permission-Camera (3.3.1):
     - RNPermissions
-  - Permission-LocationAccuracy (3.6.1):
+  - Permission-LocationAccuracy (3.3.1):
     - RNPermissions
-  - Permission-LocationAlways (3.6.1):
+  - Permission-LocationAlways (3.3.1):
     - RNPermissions
-  - Permission-LocationWhenInUse (3.6.1):
+  - Permission-LocationWhenInUse (3.3.1):
     - RNPermissions
   - Plaid (2.5.1)
   - PromisesObjC (2.1.1)
@@ -472,7 +472,7 @@ PODS:
     - React-Core
   - react-native-image-manipulator (1.0.5):
     - React
-  - react-native-image-picker (4.10.1):
+  - react-native-image-picker (4.10.2):
     - React-Core
   - react-native-netinfo (8.3.1):
     - React-Core
@@ -590,7 +590,7 @@ PODS:
     - RNFBApp
   - RNGestureHandler (2.6.0):
     - React-Core
-  - RNPermissions (3.6.1):
+  - RNPermissions (3.3.1):
     - React-Core
   - RNReactNativeHapticFeedback (1.14.0):
     - React-Core
@@ -920,7 +920,7 @@ SPEC CHECKSUMS:
   Airship: 4657c3d5118441240e04674d9445cbd6e363c956
   boost: a7c83b31436843459a1961bfd74b96033dc77234
   CocoaAsyncSocket: 065fd1e645c7abab64f7a6a2007a48038fdc6a99
-  DoubleConversion: 5189b271737e1565bdce30deb4a08d647e3f5f54
+  DoubleConversion: 831926d9b8bf8166fd87886c4abab286c2422662
   FBLazyVector: 4c04f10d8958a7be74dc063bc0584ff223b89d4a
   FBReactNativeSpec: b6ac4163edc4db460a21e2de83c8a9f3acd5c6cc
   Firebase: 629510f1a9ddb235f3a7c5c8ceb23ba887f0f814
@@ -942,7 +942,7 @@ SPEC CHECKSUMS:
   Flipper-RSocket: d9d9ade67cbecf6ac10730304bf5607266dd2541
   FlipperKit: cbdee19bdd4e7f05472a66ce290f1b729ba3cb86
   fmt: ff9d55029c625d3757ed641535fd4a75fedc7ce9
-  glog: 04b94705f318337d7ead9e6d17c019bd9b1f6b1b
+  glog: 5337263514dd6f09803962437687240c5dc39aa4
   GoogleAppMeasurement: 5ba1164e3c844ba84272555e916d0a6d3d977e91
   GoogleDataTransport: 1c8145da7117bd68bbbed00cf304edb6a24de00f
   GoogleUtilities: bad72cb363809015b1f7f19beb1f1cd23c589f95
@@ -953,10 +953,10 @@ SPEC CHECKSUMS:
   Onfido: bdbc3ed45598aa106ab2ea021d94e2e28c6b5be3
   onfido-react-native-sdk: 5856e76fbfc0eb7b70b0f76fa1059830932a5c88
   OpenSSL-Universal: ebc357f1e6bc71fa463ccb2fe676756aff50e88c
-  Permission-Camera: bf6791b17c7f614b6826019fcfdcc286d3a107f6
-  Permission-LocationAccuracy: 76df17de5c6b8bc2eee34e61ee92cdd7a864c73d
-  Permission-LocationAlways: 8d99b025c9f73c696e0cdb367e42525f2e9a26f2
-  Permission-LocationWhenInUse: 3ba99e45c852763f730eabecec2870c2382b7bd4
+  Permission-Camera: bae27a8503530770c35aadfecbb97ec71823382a
+  Permission-LocationAccuracy: 43d7a6b3d736a90168e9fc7117a721ca44722f83
+  Permission-LocationAlways: fbf36cbdcabc3101f1168ad1ec45cfc5a6448cc4
+  Permission-LocationWhenInUse: 006c85c8de0c05b5d8be8e8029e4f6b813270293
   Plaid: 6beadc0828cfd5396c5905931b9503493bbc139a
   PromisesObjC: ab77feca74fa2823e7af4249b8326368e61014cb
   RCT-Folly: 0080d0a6ebf2577475bda044aa59e2ca1f909cda
@@ -980,7 +980,7 @@ SPEC CHECKSUMS:
   react-native-document-picker: f68191637788994baed5f57d12994aa32cf8bf88
   react-native-flipper: dc5290261fbeeb2faec1bdc57ae6dd8d562e1de4
   react-native-image-manipulator: c48f64221cfcd46e9eec53619c4c0374f3328a56
-  react-native-image-picker: f2ab1215d17bcfe27b0eb6417cc236fd1f4775e7
+  react-native-image-picker: bf34f3f516d139ed3e24c5f5a381a91819e349ea
   react-native-netinfo: 1a6035d3b9780221d407c277ebfb5722ace00658
   react-native-pdf: 33c622cbdf776a649929e8b9d1ce2d313347c4fa
   react-native-plaid-link-sdk: 77052f329310ff5a36ddda276793f40d27c02bc4
@@ -1011,7 +1011,7 @@ SPEC CHECKSUMS:
   RNFBCrashlytics: 2061ca863e8e2fa1aae9b12477d7dfa8e88ca0f9
   RNFBPerf: 389914cda4000fe0d996a752532a591132cbf3f9
   RNGestureHandler: 920eb17f5b1e15dae6e5ed1904045f8f90e0b11e
-  RNPermissions: dcdb7b99796bbeda6975a6e79ad519c41b251b1c
+  RNPermissions: 34d678157c800b25b22a488e4d8babb57456e796
   RNReactNativeHapticFeedback: 1e3efeca9628ff9876ee7cdd9edec1b336913f8c
   RNReanimated: 069f3aff5df4cbefaf81589c0622370073a89f1d
   RNScreens: 0df01424e9e0ed7827200d6ed1087ddd06c493f9

Which, ignoring a few unrelated package changes, does have the updated references to the new package version 3.3.1.

So I do think your idea of finding the package version in package.json and ensuring it matches in Podfile.lock makes sense (and also thanks for linking the autolinking docs)!

[roryabraham]

I would definitely support this check. Let's add it to verifyPodfile.sh. It seems simple enough to run an npm install and pod install in a CI runner and then see if there's a diff in Podfile.lock. 👍🏼

[melvin-bot[bot]]

Current assignee @JmillsExpensify is eligible for the External assigner, not assigning anyone new.

[melvin-bot[bot]]

Job added to Upwork: https://www.upwork.com/jobs/~01baa80d227834e4fb

[melvin-bot[bot]]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @mollfpr (External)

[melvin-bot[bot]]

Current assignee @amyevans is eligible for the External assigner, not assigning anyone new.

[JmillsExpensify]

Just for clarity, is this issue open for external proposals now?

[lordthien]

This are our setup of Cocoapods, in our iOS App:

1. Commit Podfile with declare exact version for it's library
2. Git ignore the Podfile.lock
3. Git ignore the folder Pods
4. Always do pod install in the beginning (before compiling the app)

[amyevans]

Just for clarity, is this issue open for external proposals now?

Yes, open for external proposals!

Git ignore the Podfile.lock

This is not a best practice so not something we will pursue.

[redstar504]

Hi there, first time contributor here, and looking forward to helping out where I can.

After analysing this issue, I believe I may have come up with a promising solution.

The command react-native config displays a list of the node modules that have native platform dependencies. It also displays the respective Podspec files contained by each of these modules. These Podspec files contain the versioning for each native dependency, and are used when determining what happens when running pod install.

What we could do is add a new GitHub action that compares the dependency portion of the react-native config command between the PR and the main branch.

First it would do a shallow comparison of the dependency list. If this fails we can hold the PR right away, as it means npm packages were added or removed without running pod install. If the shallow comparison succeeds, we would do a comparison of the SHA1 sum of each Podspec file between the PR and the main branch to account for package updates.

This should be a fairly performant operation, without having to depend on a MacOS runner and the overhead of installing the pods themselves during CI.

[mollfpr]

@redstar504 Thanks for the proposal and clear explanation! If possible, could you provide the changes we should make?

First it would do a shallow comparison of the dependency list. If this fails we can hold the PR right away, as it means npm packages were added or removed without running pod install.

It's still unclear to me what we compare the dependencies with. Could you elaborate more?

[redstar504]

I've found that running pod install on a MacOS runner is rather slow, so I've been trying to come up with a way to avoid that if possible.

Using the react-native config command, we can see which node packages require pods. We can compare the output of this command between the PR and the main branch to determine if the pod dependencies have changed as a result of the PR. First we could compare the size of the dependency key to assert whether pods were added or removed. Then we could compare the versions to determine whether there were upgrades or downgrades. This by itself doesn't confirm whether pod install has been run, but we can use it as a prerequisite to running one of the following checks:

1. We could simply check whether Podfile.lock has been modified as well. If it hasn't, and our check above confirms there's been a change to pod dependencies, then we know the developer never ran pod install after installing packages that changed the pod requirements. This only covers the basic scenario. If they ran pod install once and then installed additional packages but forgot to run it the second time, this would provide a false-positive.
2. We could use @roryabraham's idea. I've determined that running pod install in a MacOS runner with the currently required packages takes approximately ten minutes. If the extra overhead isn't a problem this is probably the way to go. Especially since we could avoid running it for every PR, only on ones that meet the criteria above.
3. We could parse the Pod names and versions from the respective Podspec files in each package, and confirm these packages are listed in Podfile.lock. This might be the most robust solution, but would require the most development effort and testing.

I'm willing to pursue any of the options above. Thoughts?

[JmillsExpensify]

@amyevans I wanted to give you a heads up that I'm OOO this week, so if there is anything you need to happen while I'm out, can you either take it or float the task in #expensify-open-source or #bug-zero? I'm going to try and keep up every couple of days, but my responses will be delayed. 😄

[mollfpr]

Thanks again for the detail @redstar504!

Using the react-native config command, we can see which node packages require pods. We can compare the output of this command between the PR and the main branch to determine if the pod dependencies have changed as a result of the PR. First we could compare the size of the dependency key to assert whether pods were added or removed. Then we could compare the versions to determine whether there were upgrades or downgrades. This by itself doesn't confirm whether pod install has been run, but we can use it as a prerequisite to running one of the following checks:

It seems a good idea to do this before doing the check, but still, I couldn't imagine how we are going to do that.

Could you please add a code snippet of your proposal? It's not mandatory but it will help us to review your proposal.

[redstar504]

Hi @mollfpr,

I have created a reusable workflow that demonstrates the task of checking that the Podfile.lock contains all of the pod dependencies (including the correct versions) as required by the node modules.

The workflow is located here on my repository: http://github.com/redstar504/react-pods-workflow

I have demonstrated a test of it in action against a forked version of the Expensify App, which in fact was not in sync at the time I forked it.

1. The first run failed because the react-native-image-picker node module had been updated sometime, but pod install had not been run since that happened: https://github.com/redstar504/ExpensifyApp/actions/runs/3693517453
2. Committing and pushing the updated Podfile.lock after running pod install, the check runs again and succeeds: https://github.com/redstar504/ExpensifyApp/actions/runs/3693587496/jobs/6253745672

The only case it doesn't currently cover is removing node modules that contain Pod dependencies. It only checks that all of the existing node_modules containing pod dependencies exist in the Podfile.lock with the correct versions. I can add this functionality if necessary, but I don't think it's as important as making sure the required Pods are added and up to date.

Note: You will see some warnings on my test runs related to a deprecated action, but they are only because I was using some older caching logic to make running npm install portion of the workflow quicker. This logic needs to be updated to be future proof, I just haven't had a chance.

[mollfpr]

Thanks, @redstar504 great job on the demonstration!

I have a couple of questions:

1. Why do we need to run npm install on the run?
2. On challengePodlock, in my understanding it only challenges new/update version dependencies. Is this will also check if we remove any dependencies?

cc @amyevans @roryabraham @AndrewGable

[redstar504]

@mollfpr

We first run npm install so that we can parse the podspec files from the packages. The podspecs are what contain the name and version of the pods that are supposed to be installed. We then simply do a lookup in Podfile.lock to make sure they're listed along with the correct version.

It doesn't currently handle the case of removing packages, but if I get accepted for the job I can add that.

[mollfpr]

@redstar504 make sense to me!

I’ll let @amyevans @roryabraham @AndrewGable to decide.

[amyevans]

I'll review before Monday (hopefully later today)!

[amyevans]

The logic of the proposal makes sense to me, and definitely appreciate you whipping up the proof of concept repo @redstar504. I think the actual implementation could use a little refinement, but we can hammer that out.

I know @roryabraham has some thoughts as well though so I'll let him drop them in.

[roryabraham]

@redstar504 One suggestion I have is that we can eliminate the collectPodspecs action and replace these steps:

- name: Set react-native config to an environment variable
  run: |
    echo 'JSON_RESPONSE<<EOF' >> $GITHUB_ENV
    ./node_modules/.bin/react-native config >> $GITHUB_ENV
    echo 'EOF' >> $GITHUB_ENV
- name: Collect .podspec files used by the project
  id: collect
  uses: redstar504/react-pods-workflow/.github/actions/collectPodspecs@main
  with:
    config: ${{ toJSON(env.JSON_RESPONSE) }}

with something a bit simpler imo:

- name: Collect .podspec files used by the project
  id: collect
  run: |
    echo "PODSPEC_FILES=$(\
      npx react-native config | \
      jq '[.dependencies[].platforms.ios.podspecPath | select( . != null )]' \
    )" >> "$GITHUB_OUTPUT"

[redstar504]

@roryabraham Good thinking! I'll get that changed on my repository and make sure it still functions as expected.

[roryabraham]

@redstar504 Another suggestion – I notice you have some code here that uses sed to get pure-JSON output from pod ipc spec. A simpler solution would be to simply run pod ipc spec --silent.

For a POC, when I run pod ipc spec /Users/rory/Expensidev/App/node_modules/@react-native-firebase/analytics/RNFBAnalytics.podspec I see some non-JSON preamble:

But if I simply append the --silent flag, I get pure JSON:

[roryabraham]

Looking more at what https://github.com/redstar504/react-pods-workflow/blob/main/.github/scripts/podspecToJson.sh does, it gets a comma-separated array of JSON data, where each object in the array is the podspec.

Combining some feedback from an earlier comment, I think we can pretty easily eliminate the need for the podspecToJSON and replace all these lines with a single step:

- name: Collect PodSpecs used by the project
  id: collect
  run: |
    echo "PODSPECS=$( \
      jq --slurp '.' <<< "$( \
        npx react-native config | \
        jq '.dependencies[].platforms.ios.podspecPath | select( . != null )' | \
        xargs -L 1 pod ipc spec --silent
      )"
    )" >> "$GITHUB_OUTPUT"

[redstar504]

@roryabraham Nice and concise! Giving me motivation to learn the jq library.

[roryabraham]

Digging in yet further, it seems like the only fields we really need are name and version. Then we just concat that into a string like ${name} (${version}).

So we should be able to adjust the above jq filter to just give us the format we need.

-      jq --slurp '.' <<< "$( \
+      jq --raw --slurp 'map((.name + " (" + .version + ")")) | .[]' <<< "$( \

That produces output like so:

So then the only remaining task is looping through that output and making sure that each line is found in the Podfile.lock. Something like this seems like it would do the trick:

PODSPECS=$( \
  jq --raw --slurp 'map((.name + " (" + .version + ")")) | .[]' <<< "$( \
    npx react-native config | \
    jq '.dependencies[].platforms.ios.podspecPath | select( . != null )' | \
    xargs -L 1 pod ipc spec --silent
  )"
)

while read -r SPEC; do
  if ! grep -q "$SPEC" ./ios/Podfile; then
    echo "ERROR: Podspec $SPEC not found in Podfile.lock. Did you forget to run \`pod install\`?"
    exit 1
  fi
done <<< "$PODSPECS"

[redstar504]

Great job with the refactor @roryabraham! That completely simplifies my solution. The only changes I found necessary were replacing the --raw flag with --raw-output, and searching the Podfile.lock rather than the Podfile.

Since we can now accomplish everything with bash, I have modified the verifyPodfile.sh script on my forked version of the app to use a macOS runner and to combine the existing Podfile check with the condensed solution. It appears to be functioning properly.

I've completed a test run on my example repository showing it both failing with a missing pod, and succeeding after the updated Podfile.lock is re-added:

1. https://github.com/redstar504/ExpensifyApp/actions/runs/3721644050/jobs/6311934999 (fail)
2. https://github.com/redstar504/ExpensifyApp/actions/runs/3721681010/jobs/6311996126 (pass)

Here is a summary of changes in an example PR on my fork: https://github.com/redstar504/ExpensifyApp/pull/3/files

[mollfpr]

Thanks, @roryabraham, and @redstar504 for the back and forth 👍

I'll give 👍 to @redstar504 proposal, with a note that we cover the removed package too.

cc @amyevans

[redstar504]

Thanks @mollfpr. Here's what I'm thinking of for checking for removals.

After the previously discussed test passes, we could checkout the package.json and package-lock.json from the main branch into the test runner's workspace. This branch can be included by using the branch filter in the checkout action. We can then run npm i again, which should be quick as there shouldn't be many changes to the dependency graph.

This would allow us to evaluate the difference between the output of the react-native config command between the feature branch and main branch. If we previously stored the feature branch's podspecs in a bash variable, we can find the podspecs that have been removed on the feature branch by using jq:

echo '{"mainBranch":["package1.podspec","package2.podspec","package3.podspec"],"featureBranch":["package1.podspec","package2.podspec"]}' | jq '.mainBranch-.featureBranch'

$ ["package3.podspec"]

We could then reuse the code that searches the Podfile.lock, and look for each podspec outputted by the subtraction. If they are found in Podfile.lock then we know pod install was not run after removing a node module.

To keep everything in bash we should be able to run git checkout main -- {package,package-lock}.json and npm i commands in the script rather than switching back into the workflow and ushering data around.

What do you think @roryabraham @amyevans ?

[amyevans]

Sorry all for the radio silence, I've been very sick this week. This is top of my list to address when I'm feeling better.

[mollfpr]

@amyevans No problem! I hope you'll be well soon.

[Maftalion]

Hey, I know this has begun to be worked on but I was just wondering why it's scoped down to just podfile.lock changes?

My proposal would be to run any env related setup (npm i, gem install, pod install) then just let git tell you if files have changed with git diff --name-only. If this outputs anything, then something wasn't correctly committed and you'll know whether you have a clean working directory or not.

The only concern would be the length of running these commands, but with proper caching it shouldn't be too bad and you can be confident with your PRs before they get merged.

[redstar504]

@Maftalion The Podfile.lock is the only concern because the Pods are the only dependencies that are getting changed inadvertently via npm. Changes to the other types of packages are always getting tracked in their respective lockfiles.

We did discuss comparing the diff of the Podfile.lock earlier in the thread, but running pod install adds a lot of overhead in the runners. I don’t think the cache would alleviate much as we’d still be required to build the dependency graph from the npm podspecs to get the up to date lockfile. And if we were to cache Pods, the runner would need to copy ~2GB from the cache which adds quite a bit of time to the build.

The solution we came up with operates on top of the existing verifyPodfile script and adds practically no overhead.

[Maftalion]

Diffs can still exist if those lock files aren't properly committed or if versions (node, npm, cocoapods, etc) are different between local & CI. But if the ask is specifically just for this use case then your solution is definitely more optimal and less overhead 👍

[JmillsExpensify]

@roryabraham Given that Amy isn't feeling well at the moment, do you feel comfortable with @redstar504's proposal? It'd be great to align on the proposal we're going with, and get that in motion now.

[roryabraham]

@redstar504's proposal looks good to me. There might be a few details to work through in the PR (especially to catch removed packages) but I've seen enough to feel comfortable with the direction we're going with it 👍

[melvin-bot[bot]]

📣 @redstar504 You have been assigned to this job by @roryabraham! Please apply to this job in Upwork and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻 Keep in mind: Code of Conduct | Contributing 📖

[roryabraham]

@redstar504 please tag me directly in the PR for review since I'll be covering for @amyevans on this issue for the time being

[JmillsExpensify]

Thanks @roryabraham!

[JmillsExpensify]

In other news, @redstar504 and @mollfpr, would both of you mind applying for the Upwork job? It's $2,000 at the moment and potentially higher if we quality for the bonus, but we'd apply that later based on when the PR is merged.

[mollfpr]

Applied, thanks @JmillsExpensify

[redstar504]

Thanks guys, I have just submitted mine as well. Hoping to have the PR ready later today.

[JmillsExpensify]

Perfect! Offers sent via Upwork in the meantime. Happy holidays.

[redstar504]

I had to work through some issues today regarding the removals but it's looking good now. I'm currently testing each phase of the validation, and PR will be inbound tomorrow.

[redstar504]

PR submitted. Happy holidays!