Skip to content

Add named params and yard docs to create_milestone method at GithubHelper #426

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 11 commits into from
Nov 17, 2022
Merged

Add named params and yard docs to create_milestone method at GithubHelper #426

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
66ccfcf
GithubHelperSpec: Add Yard Docs and Named Parameters to create_milest…
raafaelima Nov 6, 2022
b658f87
CreateNewMilestoneAction: Adds named parameters to create_milestone G…
raafaelima Nov 6, 2022
10528de
GithubHelper#create_milestone: Rephrase yard docs
raafaelima Nov 6, 2022
255d108
GithubHelper#create_milestone: Rename parameters to be more descriptive
raafaelima Nov 7, 2022
99a95ac
CreateNewMilestoneAction: Rephrase comment about release times to be …
raafaelima Nov 8, 2022
39b23c8
GithubHelper#create_milestone: Add unit tests and validation to date …
raafaelima Nov 8, 2022
0750081
Merge branch 'trunk' into add/named-params-create-milestone
raafaelima Nov 14, 2022
b218ccc
GithubHelper: Fix Timezone changes unit tests
raafaelima Nov 14, 2022
de90d4d
GithubHelper: Adds more unit tests to validate the Timezone changes
raafaelima Nov 14, 2022
d87c03c
GithubHelper: Fix Rubocop Issues
raafaelima Nov 16, 2022
7f99d51
GithubHelperSpec: Adds comments to DST cases to be more clear and new…
raafaelima Nov 16, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 14 additions & 1 deletion lib/fastlane/plugin/wpmreleasetoolkit/actions/common/create_new_milestone_action.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,14 +11,27 @@ def self.run(params)

github_helper = Fastlane::Helper::GithubHelper.new(github_token: params[:github_token])
last_stone = github_helper.get_last_milestone(repository)

UI.message("Last detected milestone: #{last_stone[:title]} due on #{last_stone[:due_on]}.")

milestone_duedate = last_stone[:due_on]
milestone_duration = params[:milestone_duration]
newmilestone_duedate = (milestone_duedate.to_datetime.next_day(milestone_duration).to_time).utc
newmilestone_number = Fastlane::Helper::Ios::VersionHelper.calc_next_release_version(last_stone[:title])
number_of_days_from_code_freeze_to_release = params[:number_of_days_from_code_freeze_to_release]
# If there is a review process, we want to submit the binary 3 days before its release
# Using 3 days is mostly for historical reasons where we release the apps on Monday and submit them on Friday.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think of this rewording?

Suggested change
# If there is a review process, we want to submit the binary 3 days before its release
# Using 3 days is mostly for historical reasons where we release the apps on Monday and submit them on Friday.
# Because of the app stores review process, we submit the binary 3 days before the intended release date.
# Using 3 days is mostly for historical reasons, for a long time, we've been submitting apps on Friday and releasing them on Monday.

days_until_submission = params[:need_appstore_submission] ? (number_of_days_from_code_freeze_to_release - 3) : milestone_duration

UI.message("Next milestone: #{newmilestone_number} due on #{newmilestone_duedate}.")
github_helper.create_milestone(repository, newmilestone_number, newmilestone_duedate, milestone_duration, number_of_days_from_code_freeze_to_release, params[:need_appstore_submission])

github_helper.create_milestone(
repository: repository,
title: newmilestone_number,
due_date: newmilestone_duedate,
days_until_submission: days_until_submission,
days_until_release: number_of_days_from_code_freeze_to_release
)
end

def self.description
Expand Down
24 changes: 14 additions & 10 deletions lib/fastlane/plugin/wpmreleasetoolkit/helper/github_helper.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,15 +71,19 @@ def get_last_milestone(repository)
last_stone
end

def create_milestone(repository, newmilestone_number, newmilestone_duedate, newmilestone_duration, number_of_days_from_code_freeze_to_release, need_submission)
# If there is a review process, we want to submit the binary 3 days before its release
#
# Using 3 days is mostly for historical reasons where we release the apps on Monday and submit them on Friday.
days_until_submission = need_submission ? (number_of_days_from_code_freeze_to_release - 3) : newmilestone_duration
submission_date = newmilestone_duedate.to_datetime.next_day(days_until_submission)
release_date = newmilestone_duedate.to_datetime.next_day(number_of_days_from_code_freeze_to_release)
# Creates a new milestone
#
# @param [String] repository The repository name, including the organization (e.g. `wordpress-mobile/wordpress-ios`)
# @param [String] title The name of the milestone we want to create (e.g.: `16.9`)
# @param [Time] due_date milestone due date (e.g. `2022-10-22T12:00:00Z`)
# @param [Integer] days_until_submission Number of days until submission
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# @param [Time] due_date milestone due date (e.g. `2022-10-22T12:00:00Z`)
# @param [Integer] days_until_submission Number of days until submission
# @param [Time] due_date milestone due date—which will also correspond to the code freeze date
# @param [Integer] days_until_submission Number of days from code freeze to submission to the App Store / Play Store

# @param [Integer] days_until_release Number of days from code freeze to release
#
def create_milestone(repository:, title:, due_date:, days_until_submission:, days_until_release:)
submission_date = due_date.to_datetime.next_day(days_until_submission)
release_date = due_date.to_datetime.next_day(days_until_release)
comment = <<~MILESTONE_DESCRIPTION
Code freeze: #{newmilestone_duedate.to_datetime.strftime('%B %d, %Y')}
Code freeze: #{due_date.to_datetime.strftime('%B %d, %Y')}
App Store submission: #{submission_date.strftime('%B %d, %Y')}
Release: #{release_date.strftime('%B %d, %Y')}
MILESTONE_DESCRIPTION
Expand All @@ -96,9 +100,9 @@ def create_milestone(repository, newmilestone_number, newmilestone_duedate, newm
#
# This is a bug in the GitHub API, not in our date computation logic.
# To solve this, we trick it by forcing the time component of the ISO date we send to be `12:00:00Z`.
options[:due_on] = newmilestone_duedate.strftime('%Y-%m-%dT12:00:00Z')
options[:due_on] = due_date.strftime('%Y-%m-%dT12:00:00Z')
options[:description] = comment
client.create_milestone(repository, newmilestone_number, options)
client.create_milestone(repository, title, options)
end

# Creates a Release on GitHub as a Draft
Expand Down
12 changes: 6 additions & 6 deletions spec/github_helper_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -234,14 +234,14 @@ def get_milestone(milestone_name:)
end

def create_milestone(need_submission:, milestone_duration:, days_code_freeze:)
days_until_submission = need_submission ? (days_code_freeze - 3) : milestone_duration
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this file is focused on testing the helper, and the helper doesn't have knowledge of the concept of need_submission anymore, I think we should adapt the github_helper_spec accordingly, to write the it tests based on the new parameters (i.e. don't test "has correct dates (with|without) submission" cases, but test things like "computes the correct dates for (a standard case | dates crossing DST boundaries | inconsistent offsets | …)".

Also begs the question what the behavior should do if we provide inconsistent inputs (invalid date, negative values for days_until_*, days_until_release < days_until_submission, …?).
Maybe it's ok to let that to the caller's responsibility and not check (i.e. keep the current behavior), at least for now; but adding tests to the action like here would also be the best time to raise this kind of questions (after all, if we try to follow in the footsteps of TDD, we should write the tests to describe the behaviors and use cases we want first, and then we implement them to match, not the other way around).

Copy link
Contributor Author

@raafaelima raafaelima Nov 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also begs the question what the behavior should do if we provide inconsistent inputs (invalid date, negative values for days_until_*, days_until_release < days_until_submission, …?). [.....]

That's a question that I keep asking myself 🤔 💭

Using as an example a case where I have more days from submission than from release (i.e days_until_submission: 14, days_until_release: 3), The submission date will be waaay after the release date. In this case, should we raise a FastlaneError?

# Milestone with due_date = '2022-10-20', days_until_submission = 14 and days_until_release = 3
Code freeze: October 20, 2022
App Store submission: November 03, 2022
Release: October 23, 2022

Also, in the cases that we have negative values for these same parameters (i.e days_until_submission: -14, days_until_release: -14), the Ruby DateApi at param.to_datetime.next_day(..) just subtracts days from the due_date, although is allowed, it is another inconsistency. So, negative values or zero should be allowed on these parameters, in the first place?

# Milestone with due_date = '2022-10-20', days_until_submission = -14 and days_until_release = -14
Code freeze: October 20, 2022
App Store submission: October 06, 2022
Release: October 06, 2022

Also, assuming that when we refer to Release, we are talking about the date when this version will be available to our users, and by AppStore Submission, is the date we submit the code to be reviewed and alpha/beta tested.

What is the expected behavior of the dates when creating milestones?

  • The Code Freeze date should always be before AppStore Submission and Release dates?
  • The Code Freeze date is always before AppStore Submission, or those can also be equal?
  • The Release date is always after AppStore Submission, or those can also be equal?
  • Should this be validated at all?

Validating dates is always tricky, I just feel that I should confirm my assumptions before starting anything 😅

I also feel that I should write a mini-P2 to document those changes, just in case anyone passes through here in the future and needs to do any refactorings. 🗒️

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep writing a P2 seems a good idea.

As for the cases you mention above, those seems to be good ones; I think we should add those, then use UI.user_error! in the implementation for when those cases are encountered.

Copy link
Contributor

@AliSoftware AliSoftware Nov 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the expected behavior of the dates when creating milestones?

Every sprint we:

  • Do the code freeze for version N on Monday, which corresponds to us cutting a release/* branch from trunk, adding ❄️ to the N milestone, pushing some new adjustments commits in the release/* (like freezing the strings to send to translation, bumping the version numbers, …), then create a first beta that we send to beta testers
  • Then we let that period of beta test on the code-frozen beta run for almost a whole sprint (while developers are working on trunk on version N+1 in parallel), doing new betas from the release/* branch during that period if there are last-minute betafixes to land
  • On the Friday at the end of the Sprint, once the beta-test period is over and we hopefully beta-fixed anything that was urgent to fix during that period, we submit the final build to the App Store / Play Store
  • On the next Monday, providing that Apple/Google have approved the build we submit over the weekend, we start the rollout of that build to end users, and start a new sprint (code freeze of version N+1; wash, rince, repeat)

This process is also described in this old blog post, though it might be a tad outdated now.

Nowadays some of our apps still have 2-weeks sprints, which means when we do a code freeze on Day D+0 (Monday, start of sprint), we then submit for review on Day D+11 (i.e. second Friday), hope it gets approved over the weekend (it usually is), and release on Day D+14 (next Monday), same day we start a new code freeze. Some other apps use 1-week sprints, which means code freeze at D+0, submission at D+4, release at D+7.

helper = described_class.new(github_token: 'Fake-GitHubToken-123')
helper.create_milestone(
test_repo,
test_milestone_number,
test_milestone_duedate.to_time.utc,
milestone_duration,
days_code_freeze,
need_submission
repository: test_repo,
title: test_milestone_number,
due_date: test_milestone_duedate.to_time.utc,
days_until_submission: days_until_submission,
days_until_release: days_code_freeze
)
end
end
Expand Down