[Tooling] Fix complete_code_freeze push on tags#17627
Conversation
|
You can trigger an installable build for these changes by visiting CircleCI here. |
|
You can trigger optional UI/connected tests for these changes by visiting CircleCI here. |
|
|
||
| UI.confirm('Ready to push changes to remote and trigger the beta build?') unless ENV['RELEASE_TOOLKIT_SKIP_PUSH_CONFIRM'] | ||
| push_to_git_remote | ||
| push_to_git_remote(tags: false) |
There was a problem hiding this comment.
I wonder why fastlane developers opted to make tags parameter true by default. That seems a bit risky to me. 🤷
There was a problem hiding this comment.
Yep I was surprised by this as well, quite unfortunate.
I'm back-and-forth on creating our own helper action (which could be something like git_commit(message:files:push:) or something like that) in the release-toolkit to have our own default values and options, also because this built-in one as that tags: true as default. Not sure if it's worth it though.
There was a problem hiding this comment.
I don't know how often this method is used, so I don't have any feedback on that. However, what you're proposing makes sense to me if you feel it'd be worthwhile.
Not so fun fact, I struggled a bit finding where this method(?) was defined. I searched for the project and release-toolkit convinced that it was supposed to be there. I only later realized it might be a fastlane method. One of many things I struggle with Ruby(?) or the release-toolkit structure where things are not very obvious for someone like me who is used to code being a bit more explicit.
There was a problem hiding this comment.
Doesn't really answer your question, but when the parameter was added, they set it to true for backward compatibility.
I think it would be reasonable to suggest to switch to false as the default. I wonder how they would treat that change from a SemVer point of view, though 🤔 Maybe there's too much inertia in the tool's adoption to introduce a breaking but little change like this
There was a problem hiding this comment.
I'm pretty sure from how the fastlane team runs things that this would be a no-no for them to change the default value of this action there, because they're pretty conservative in the API, oftentimes preferring to keep an old API intact despite finding flaws in it later, rather than doing a breaking change.
Since it was created and reach its first stable version (1.0.0) years, fastlane only made a major version (2.x.x) once (!), especially because of that and the fact they really seem to try very hard to avoid doing any breaking changes (in order to not have users make migrations every N months or so). So changing the default value here is very likely out of the question for them.
Not so fun fact, I struggled a bit finding where this method(?) was defined. I searched for the project and release-toolkit convinced that it was supposed to be there. I only later realized it might be a fastlane method. One of many things I struggle with Ruby(?) or the release-toolkit structure where things are not very obvious for someone like me who is used to code being a bit more explicit.
💯 I wish Fastlane had at least some namespacing concept, so that we could do a8c.some_action to distinguish our actions from the Release Toolkit from the rest. Or even better, an ability to have nested namespaces like a8c.ios.some_action, a8c.ios.git.some_sub_action… That would help clarify a lot. Sadly I don't think that's a feature they currently support 😒
PS: Note that Fastlane now supports writing Fastfiles in Swift (as an alternative to Ruby). Which might (?) provide us with nicer type checking and API in our Fastfiles (though given the dynamic nature of the available actions with the plugin system, not sure how much it would improve things in practice…). I am guessing that this means depending on the presence of the Swift compiler (Xcode or the CommandLine Tools) being installed on the Mac running fastlane, which is ok for iOS projects but not an ideal for Android projects…
This trivial change in our
Fastfileaims to fix wordpress-mobile/release-toolkit#314The issue in wordpress-mobile/release-toolkit#314 can only happen if the user of the lane has invalid tags locally (i.e. local tags that are not in sync with remote tags anymore). But since we don't really need to push tags on code freeze (we only pushed them because this is the default for this fastlane action and we didn't override it), better just not push them and avoid the risk.
PS: Fixing on
developbecause even if it's fixing the release lanes, this fix only affectscomplete_code_freezewhich won't be called again during the currentrelease/18.8branch so no point in fixing that in the frozen branch as I often do otherwise.