[supply] Fix #28995 and release_status vs rollout parameters
#29484
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
AliSoftware
commented
Mar 10, 2025
Comment on lines
+565
to
+572
| context 'when release_status is IN_PROGRESS' do | ||
| include_examples 'updates track with correct status and rollout', | ||
| rollout: 1.0, | ||
| release_status: Supply::ReleaseStatus::IN_PROGRESS, | ||
| # We want to ensure the implementation forces status of COMPLETED when explicit rollout = 1.0 is provided | ||
| expected_status: Supply::ReleaseStatus::COMPLETED, | ||
| expected_user_fraction: nil | ||
| end |
There was a problem hiding this comment.
This should cover the case / request from @mcfarljw in #28995 (comment)
AliSoftware
commented
Mar 10, 2025
Comment on lines
+546
to
+553
| context 'when release_status is COMPLETED' do | ||
| include_examples 'updates track with correct status and rollout', | ||
| rollout: 0.5, | ||
| release_status: Supply::ReleaseStatus::COMPLETED, | ||
| # We want to ensure the implementation forces status of IN_PROGRESS when explicit rollout < 1.0 is provided | ||
| expected_status: Supply::ReleaseStatus::IN_PROGRESS, | ||
| expected_user_fraction: 0.5 | ||
| end |
There was a problem hiding this comment.
This is the particular case reported in #28995 we want to fix
AliSoftware
commented
Mar 10, 2025
[The API docs](https://googleapis.dev/ruby/google-api-client/latest/Google/Apis/AndroidpublisherV3/TrackRelease.html#user_fraction-instance_method) explicitly mention that the `user_fraction` parameter is only valid when `status` is `inProgress` or `halted`
release_status vs rollout parameters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Checklist
bundle exec rspecfrom the root directory to see all new and existing tests passbundle exec rubocop -ato ensure the code style is validci/circlecibuilds in the "All checks have passed" section of my PR (connect CircleCI to GitHub if not)I've updated the documentation if necessary.Motivation and Context
After I introduced this change to supply in
2.226.0, this broke the behavior for some users in #28995.This is because when they provided a
rollout:parameter with an <1.0 value, but didn't provide an explicitrelease_status:parameter (and it thus defaulted to'completed'), the new implementation made the API call withrelease_status: 'completed', user_fraction: rolloutinstead of usinginProgressimplicitly for rollout values <1.0The before/after and history of past implementations
Before https://github.com//issues/28995, it was ok to not change the `release.status` of the existing track and status that was fetched by the API to be modified by `#update_rollout`, because before that PR we only fetched releases that were in `inProgress` state, so we were guaranteed that the `status` property of those `release` objects we got back already had the `release.status = 'inProgress'`, and all the old implementation was doing was thus to update that `release.status` to `'completed'` only if a value of `rollout: '1.0'` was provided, leaving the value unchanged to `'inProgress'` otherwise.But after #28995 landed, the
releaseobjects we got back from the API could be eitherinProgressordraft. This is why that part of the implementation / logic was changed to make sure the provided value for therelease_statusparameter was applied, so that it could be changed fromdrafttoinProgressand vice-versa too if that's what the caller explicitly asked. But that implementation failed to account for the fact that therelease_statusparameter (ConfigItem) of supply had a default value of'completed'which means that if the user didn't provide arelease_statusparameter explicitly, the new code would attempt to set it to'completed'even ifrollout:was provided with a <1.0 value.Description
This new implementation was written by writing the Unit Tests (rspecs) first, to write down all the possible use cases that we could encounter with regards to combinations of
release_status/rolloutparameter pairs and write down the expectation we have for each of those cases. Then I adjusted the implementation to make sure those tests passed (TDD FTW!)Here's the various use cases covered by the unit tests and the behavior we implemented:
rolloutparameterrelease_statusparameteruser_fractionsent to API1release.statussent to APInilDRAFTnilDRAFTnilIN_PROGRESSnilCOMPLETEDnilCOMPLETED0.5DRAFTnil1DRAFT0.5IN_PROGRESS0.5IN_PROGRESS0.5COMPLETED0.5IN_PROGRESS21.0DRAFTnil1DRAFT1.0IN_PROGRESSnil1COMPLETED31.0COMPLETEDnil1COMPLETEDTesting Steps
Gemfileto the branch of this PRupload_to_play_storein yourFastfilewith the usual parameters you'd call it with, but testing the different use cases provided in the table above forrolloutandrelease_statusIn particular, it'd be nice if some of you could test those calls while your tracks in Google Play are in different states, like the track already containing a release but being in draft, or a track containing a release which has started rollout but not at 100% yet, multiple releases in draft in the same track, …
cc @Noorrama @sreejithbnaick @lberaldodev @GiebelGobbler @mcfarljw — who chimed in on the original issue #28995
Footnotes
The
user_fractionparameter is only expected by the API ifstatusisinProgressorhalted↩ ↩2 ↩3 ↩4 ↩5We intentionally force the status sent to the API to be
'inProgress'even if the user called the action with'completed', because therolloutparameter was explicitly passed with value1.0↩We intentionally force the status sent to the API to be
'completed'even if the user called the action with'inProgress', because therolloutparameter was explicitly passed but with a value <1.0 ↩