Refactor ios_check_beta_deps action to make it more resilient & detect Pods referenced by commit/branch
#557
Conversation
Allow detection of Pods referenced by commits and branches Make detection more resilient by using Podfile.lock instead of Podfile
AliSoftware
changed the title
ios_check_beta_deps action to make it more resilientios_check_beta_deps action to make it more resilient & detect Pods referenced by commit/branch
lib/fastlane/plugin/wpmreleasetoolkit/actions/ios/ios_check_beta_deps.rb
Outdated
Show resolved
Hide resolved
| non_stable_pods = {} # Key will be pod name, value will be reason for flagging | ||
|
|
||
| # Find pods referenced by commit and branch to a repo | ||
| yaml['EXTERNAL SOURCES'].each do |pod, options| |
There was a problem hiding this comment.
I just realized that some Podfile.lock files might not have this section at all (if they don't have any external dependency), so better cover for the nil case.
| yaml['EXTERNAL SOURCES'].each do |pod, options| | |
| yaml['EXTERNAL SOURCES']&.each do |pod, options| |
Ideally we should add a spec example to cover that too—the one from Simplenote-macOS can serve as a fixture for that case.
There was a problem hiding this comment.
@spencertransier I've fixed this (and added test coverage for it) via dc5992f + b1a13d9, so PR is now ready for review again 🙇
| ALL_PODS_STABLE_MESSAGE = 'All pods are pointing to a stable version. You can continue with the code freeze.'.freeze | ||
| NON_STABLE_PODS_MESSAGE = "Please create a new stable version of those pods and update the Podfile to the newly released version before continuing with the code freeze:\n".freeze |
There was a problem hiding this comment.
Why declare these constants here instead of at the top of the run method?
There was a problem hiding this comment.
While we couldn't move them at the top of the run method, I've moved them at the top of the class definition in 25420dd, at least
There was a problem hiding this comment.
Ah, got it. Thank you!
Generated by 🚫 Danger |
And move test about invalid regex closer to other regex-related tests
| ALL_PODS_STABLE_MESSAGE = 'All pods are pointing to a stable version. You can continue with the code freeze.'.freeze | ||
| NON_STABLE_PODS_MESSAGE = "Please create a new stable version of those pods and update the Podfile to the newly released version before continuing with the code freeze:\n".freeze |
There was a problem hiding this comment.
Nitpick. These messages assume the action runs as part of the code freeze. That is true in practice, and the action name sort of suggests it, too. Still, I'd suggest removing the code freeze mention to make it agnostic.
| regex = begin | ||
| Regexp.new(version_pattern) | ||
| rescue RegexpError | ||
| UI.user_error!("Invalid regex pattern: `#{version_pattern}`") |
There was a problem hiding this comment.
Side note. I wanted to share appreciation for this kind of error handling where the things that's wrong is clearly reported in the error message. Recently, I've bump my head against some vague error messages; seeing this is encouraging. Thanks!
What does it do?
Note
See discussion in p1712630806132789-slack-CC7L49W13
-beta, but not pods referenced by commit/branch)Podfile.lockinstead of thePodfile, thus addressing Makeios_check_beta_depsusePodfile.lock#216buildkite_annotate(message: result[:message])to make the warning more visible in Buildkite.Testing
Gemfileofwoocommerce-iosto point therelease-toolkitto this branchbundle install && bundle exec fastlane run ios_check_beta_depsPodfile.lockwordpress-iosrepo, and got a green light:Checklist before requesting a review
bundle exec rubocopto test for code style violations and recommendationsspecs/*_spec.rb) if applicablebundle exec rspecto run the whole test suite and ensure all your tests passCHANGELOG.mdfile to describe your changes under the appropriate existing###subsection of the existing## Trunksection.MIGRATION.mdfile to describe how the changes will affect the migration from the previous major version and what the clients will need to change and consider.