Skip to content
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.

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

First draft of running the I18n Sync Down & Out periodically throughout the week. #36750

Merged
merged 6 commits into from
Sep 17, 2020
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
21 changes: 17 additions & 4 deletions bin/i18n/sync-all.rb
Original file line number Diff line number Diff line change
Expand Up @@ -40,14 +40,14 @@ def initialize(args)

def run
if @options[:interactive]
checkout_staging
return_to_staging_branch
sync_in if should_i "sync in"
sync_up if should_i "sync up"
CreateI18nPullRequests.in_and_up if @options[:with_pull_request] && should_i("create the in & up PR")
sync_down if should_i "sync down"
sync_out(true) if should_i "sync out"
CreateI18nPullRequests.down_and_out if @options[:with_pull_request] && should_i("create the down & out PR")
checkout_staging
return_to_staging_branch
elsif @options[:command]
case @options[:command]
when 'in'
Expand Down Expand Up @@ -136,8 +136,21 @@ def should_i(question)
end
end

def checkout_staging
return if GitUtils.current_branch == "staging"
def return_to_staging_branch
case GitUtils.current_branch
when "staging"
# If we're already on staging, we don't need to bother
return
when /^i18n-sync/
# If we're on an i18n sync branch, only return to staging if the branch
# has been merged.
return unless GitUtils.current_branch_merged_into? "staging"
else
# If we're on some other branch, then we're in some kind of weird state,
# so error out.
raise "Tried to return to staging branch from unknown branch #{GitUtils.current_branch.inspect}"
end

`git checkout staging` if should_i "switch to staging branch"
end
end
Expand Down
18 changes: 17 additions & 1 deletion experimental/i18n-dev.crontab
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,25 @@
PATH=/home/ubuntu/.rbenv/shims:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin
HOME=/home/ubuntu

# Run a CI Build to update the server from staging. We do this primarily to
# make sure the server gets seeded with latest level content for the sync in.
# Note that the CI_ONLY_BUILD environment variable is necessary; the server is
# not set up to actually serve a version of the website, and it will fail to
# build correctly if not thus restricted.
0 */4 * * * cd /home/ubuntu/code-dot-org && CI_ONLY_BUILD=true bundle exec ./bin/cronjob ./aws/ci_build >>/tmp/cronlog 2>&1
Copy link
Contributor

Choose a reason for hiding this comment

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

Will this run if we're not on the staging branch? Notably, I'm worried about a situation where we run the sync on Tuesday, never switch back to the staging branch, then the Monday sync is several days out of date.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh, whoops I forgot to commit that change! One moment

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahh the continuous sync doesn't switch branches. That still means it won't be able to pull from staging if there are conflicting files but I guess that's just something to look out for.

Copy link
Contributor Author

@Hamms Hamms Sep 16, 2020

Choose a reason for hiding this comment

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

That's correct! The idea is that we progress through the branches like this:

  1. Start on staging branch. From here, we will run the CI build to update our sources files from staging latest, and also run the continuous sync to update our translation files from Crowdin.
  2. Sunday evening, we run the I18n sync, which switches us to the i18n sync branch. From here we can continue to run the continuous sync, but won't run the CI build.
  3. Once the sync PRs have been merged, we switch back to the staging branch, which now includes the latest sync changes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the explanation! And of course the i18n down/out steps don't change any English files so the chance of conflict is low.


# Run the I18n Sync!
# Sync is scheduled for 12:30 AM PST every Monday; run around midnight so the
# sync is hopefully finished by start of day on Monday, offset by 30 minutes so
# we don't conflict with the ci_build which runs on the hour.
30 8 * * 1 (cd /home/ubuntu/code-dot-org && (bundle exec ./bin/i18n/sync-all.rb -yip | ./bin/i18n/slack_log) 2>&1 | ./bin/i18n/slack_error) >>/tmp/cronlog 2>&1
30 8 * * MON (cd /home/ubuntu/code-dot-org && (bundle exec ./bin/i18n/sync-all.rb -yip | ./bin/i18n/slack_log) 2>&1 | ./bin/i18n/slack_error) >>/tmp/cronlog 2>&1

# Run the sync down periodically throughout the week. Specifically:
# - Run it every other hour. This is a somewhat arbitrarily-chosen frequency
# for a test period. We expect to refine this over time.
# - Run it on the half-hour. Like for the full sync, this is done to reduce the
# chance of colliding with the CI Build.
# - Run it every day except Monday. This is done so we don't conflict with the
# full sync, and also because we expect to spend Monday verifiying the
# results of the full sync.
30 */2 * * SUN,TUE-SAT cd /home/ubuntu/code-dot-org && ((bundle exec ./bin/i18n/sync-all -c down && bundle exec ./bin/i18n/sync-all -c out) | ./bin/i18n/slack_log) 2>&1 | ./bin/i18n/slack_error
17 changes: 17 additions & 0 deletions lib/cdo/git_utils.rb
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,23 @@ def self.current_branch
`git rev-parse --abbrev-ref HEAD`.strip
end

# Get a list of the names of all branches merged into the given branch
def self.merged_branches(base_branch)
merged_branches = `git branch --merged #{base_branch}`.split("\n")
merged_branches.each do |branch|
# The return format here is a bit messy, so in addition to splitting by
# newline to turn it into a list we also want to make sure to clean up
# whitespace and the "* " string used to denote the current branch.
branch.strip!
branch.delete_prefix! "* "
end
return merged_branches
end

def self.current_branch_merged_into?(base_branch)
merged_branches(base_branch).include? current_branch
end

def self.get_latest_commit_merged_branch
get_branch_commit_merges(git_revision)
end
Expand Down
19 changes: 19 additions & 0 deletions lib/test/cdo/test_git_utils.rb
Original file line number Diff line number Diff line change
Expand Up @@ -31,4 +31,23 @@ def test_dashboard_globs
)
assert_equal 0, files.length, 'ignores files in dashboard/test/ui'
end

def test_current_branch_merged_into
GitUtils.stubs(:current_branch).returns("current_branch")
GitUtils.stubs(:merged_branches).returns(
%w(
staging
some_other_branch
)
)
refute GitUtils.current_branch_merged_into?("staging")
GitUtils.stubs(:merged_branches).returns(
%w(
staging
current_branch
some_other_branch
)
)
assert GitUtils.current_branch_merged_into?("staging")
end
end