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

Commit lock author #901

Merged
merged 9 commits into from May 8, 2019

Conversation

Projects
None yet
3 participants
@davidcornu
Copy link
Member

commented May 2, 2019

Implements #860.

image

@davidcornu davidcornu self-assigned this May 2, 2019

@davidcornu davidcornu changed the title [WIP] Lock author [WIP] Commit lock author May 2, 2019

@davidcornu davidcornu force-pushed the lock-author branch 2 times, most recently from a570b23 to 5458ffe May 2, 2019

@@ -76,6 +76,7 @@
t.string "pull_request_title", limit: 1024
t.integer "pull_request_id"
t.boolean "locked", default: false, null: false
t.integer "lock_author_id", limit: 4

This comment has been minimized.

Copy link
@davidcornu

davidcornu May 2, 2019

Author Member

Done to match

t.integer "lock_author_id", limit: 4

@davidcornu davidcornu changed the title [WIP] Commit lock author Commit lock author May 2, 2019

@davidcornu davidcornu requested a review from casperisfine May 2, 2019

@davidcornu davidcornu force-pushed the lock-author branch from 5458ffe to 66542e4 May 2, 2019

@davidcornu

This comment has been minimized.

Copy link
Member Author

commented May 2, 2019

Fixed the Rubocop violations

@casperisfine
Copy link
Contributor

left a comment

❤️

@casperisfine

This comment has been minimized.

Copy link
Contributor

commented May 2, 2019

Wait, there is another code path, that might need change.

@casperisfine

This comment has been minimized.

Copy link
Contributor

commented May 2, 2019

@davidcornu Here the other path that would need author handling:

if commits.where(id: commits_to_lock.map(&:id).uniq).update_all(locked: true) > 1

@davidcornu davidcornu force-pushed the lock-author branch from 66542e4 to 868398f May 3, 2019

@@ -239,7 +239,7 @@ def status
:default
end

def lock_reverted_commits!
def lock_reverted_commits!(user = AnonymousUser.new)

This comment has been minimized.

Copy link
@davidcornu

davidcornu May 3, 2019

Author Member

There's another code path that uses this

new_commits, shared_parent = fetch_missing_commits { @stack.github_commits }
@stack.transaction do
shared_parent&.detach_children!
appended_commits = new_commits.map do |gh_commit|
append_commit(gh_commit)
end
@stack.lock_reverted_commits! if appended_commits.any?(&:revert?)

I left that one as commits being locked as a result of syncing with GH don't have an obvious lock author.

This comment has been minimized.

Copy link
@casperisfine

casperisfine May 3, 2019

Contributor

syncing with GH don't have an obvious lock author.

I would have used the revert author, but both are fine.

This comment has been minimized.

Copy link
@davidcornu

davidcornu May 3, 2019

Author Member

🤔 Given appended_commits.any?(&:revert?), there could be multiple no?

This comment has been minimized.

Copy link
@casperisfine

casperisfine May 3, 2019

Contributor

In SyncJob yes, but here:

if revert = backlog.shift
commits_to_lock += backlog.reverse.drop_while { |c| !revert.revert_of?(c) }

It's not ambiguous.

@davidcornu davidcornu force-pushed the lock-author branch from 868398f to 831d218 May 3, 2019

@davidcornu davidcornu force-pushed the lock-author branch 2 times, most recently from 5b945ec to be75dbe May 3, 2019

@@ -240,21 +240,23 @@ def status
end

def lock_reverted_commits!

This comment has been minimized.

Copy link
@davidcornu

davidcornu May 3, 2019

Author Member

@casperisfine thoughts on this change? I refactored the logic to lock in batches rather than all at once so the lock_author can be set to the revert commit author.

Tests are gonna fail. Will fix after work.

This comment has been minimized.

Copy link
@casperisfine

casperisfine May 3, 2019

Contributor

LGTM. I'd use affected_rows rather than affected though /nitpick.

@davidcornu davidcornu force-pushed the lock-author branch from 8b59e89 to 22403f4 May 3, 2019

@davidcornu davidcornu requested a review from casperisfine May 3, 2019

@@ -729,5 +730,105 @@ def setup

@stack.async_refresh_deployed_revision
end

test "#lock_reverted_commits! locks all commits between the original and reverted commits" do

This comment has been minimized.

Copy link
@davidcornu

davidcornu May 3, 2019

Author Member

Ended up adding a bunch of test coverage here as there's some in

test "#perform locks all commits leading to a revert" do
@stack.deploys_and_rollbacks.destroy_all
initial_queue = [
["whoami", false],
["fix all the things", false],
["yoloshipit!", false],
["fix it!", false],
["sheep it!", false],
["lets go", false],
]
assert_equal initial_queue, @stack.undeployed_commits.map { |c| [c.title, c.locked?] }
author = stub(
id: 1234,
login: 'bob',
name: 'Bob the Builder',
email: 'bob@bob.com',
date: '2011-04-14T16:00:49Z',
)
@job.expects(:fetch_missing_commits).returns([
[
stub(
sha: '36514755579bfb5bc313f403b216f4347a027990',
author: author,
committer: author,
stats: nil,
commit: stub(
sha: '36514755579bfb5bc313f403b216f4347a027990',
message: 'Revert "fix it!"',
author: author,
committer: author,
),
),
],
shipit_commits(:fifth),
])
@job.perform(stack_id: @stack.id)
final_queue = [
['Revert "fix it!"', false],
["fix all the things", true],
["yoloshipit!", true],
["fix it!", true],
["sheep it!", false],
["lets go", false],
]
assert_equal final_queue, @stack.reload.undeployed_commits.map { |c| [c.title, c.locked?] }
end
and
test "when a rollback succeed reverted commits are locked" do
@stack.tasks.where.not(id: shipit_tasks(:shipit_complete).id).delete_all
deploy = @stack.deploys.success.last
reverted_commit = deploy.until_commit
@stack.commits.create!(
sha: '50ce7d4440fcd8c734f8b7b76c86f8db46706e4f',
message: %(Revert "#{reverted_commit.message_header}"),
author: reverted_commit.author,
committer: reverted_commit.committer,
authored_at: Time.zone.now,
committed_at: Time.zone.now,
)
expected = [
['Revert "Merge pull request #7 from shipit-engine/yoloshipit"', false, nil],
["whoami", false, nil],
['fix all the things', false, nil],
]
assert_equal(expected, @stack.undeployed_commits.map { |c| [c.title, c.locked?, c.lock_author_id] })
rollback = deploy.trigger_revert
rollback.run!
rollback.complete!
user_id = reverted_commit.author.id
expected = [
['Revert "Merge pull request #7 from shipit-engine/yoloshipit"', false, nil],
["whoami", true, user_id],
['fix all the things', true, user_id],
['yoloshipit!', true, user_id],
]
assert_equal(expected, @stack.undeployed_commits.map { |c| [c.title, c.locked?, c.lock_author_id] })
end
but not enough to convince me that I wasn't breaking anything with the refactor.

I've also split the addition of these tests and the refactor into separate commits so it's more obvious what my changes actually did.

@davidcornu davidcornu requested a review from DazWorrall May 7, 2019

@DazWorrall
Copy link
Member

left a comment

This looks great, thanks for stepping up

@davidcornu davidcornu merged commit 033ea77 into master May 8, 2019

3 checks passed

CLA Contributor License Agreement (CLA) status
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@davidcornu davidcornu deleted the lock-author branch May 8, 2019

@davidcornu davidcornu referenced this pull request May 8, 2019

Closed

Show who locked a commit #860

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.