woocommerce / woocommerce-ios

WooCommerce iOS app
https://www.woocommerce.com/mobile
GNU General Public License v2.0
258 stars 108 forks source link

[Tooling] Fix GitHub releases for intermediate betas #4404

Closed AliSoftware closed 2 years ago

AliSoftware commented 2 years ago

Why?

Similar PR than WPiOS's https://github.com/wordpress-mobile/WordPress-iOS/pull/16674

Fix the GitHub releases drafts being incorrect for intermediate betas:

To Test

Not easy to test from start to finish without creating a new beta… and we already did the finalize_release for this Sprint so we should not create a new beta even just for testing, so not sure if there's a way to dry-run/test this 😒

Best way I can think of is comment all the steps in the Fastfile that have side effects on build_and_upload_itc (namely gym, testflight and sentry) and trigger the lane with the same parameters that are used in the .circleci/config.yml for beta build, and check the result. But that will only test a tiny portion of the changes, like it won't test that the release notes extraction works to use the right release notes for first betas nor that the round trip from "run lane on your Mac -> trigger CI -> CI run its own lane" doesn't break mid-chain…

So maybe the best way to test this besides just reviewing the code is to test this during the next cycle…

peril-woocommerce[bot] commented 2 years ago
Warnings
:warning: PR has more than 500 lines of code changing. Consider splitting into smaller PRs if possible.

Generated by :no_entry_sign: dangerJS

peril-woocommerce[bot] commented 2 years ago

You can trigger optional UI/connected tests for these changes by visiting CircleCI here.

peril-woocommerce[bot] commented 2 years ago

You can trigger an installable build for these changes by visiting CircleCI here.

jkmassel commented 2 years ago

Let's try to test this one this cycle (following the code freeze)

AliSoftware commented 2 years ago

@jkmassel feel free to take over the PR and push Gio's suggestions and rebase this after 6.9 code freeze 🙂

shiki commented 2 years ago

Moving this to the 7.0 milestone. :)

AliSoftware commented 2 years ago

@shiki as @jkmassel said above I think the plan is to actually rebase this onto release/6.9 and merge this after code freeze during this sprint, so that Jeremy can test it next during the beta cycle 😅

shiki commented 2 years ago

Lol sorry. I don't know how to read.

jkmassel commented 2 years ago

No merge issues rebasing to release/6.9!

If we need another beta this week, I'll test this PR then. If not, I'll test this PR on its own early next week in a throwaway beta build.

AliSoftware commented 2 years ago

@jkmassel I believe you made a beta recently, were you able to test this fix?

Even if not I think it's worth merging sooner than later because:

AliSoftware commented 2 years ago

@loremattei this was split in two PRs, with the styling separated in #4417 and the fix only in this PR.

But what happened is that #4417 got then merged before this PR, and this made the diff of this PR change and include the changes from #4417, hence the bigger diff when you finally reviewed, while the original intent was to review (and potentially merge) this PR first and #4417 next, which would have kept the diffs separate as it originally was and as intended.