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

Spring Cleaning IS-4644 Enable use of auto-merging resolver #83

Merged
merged 7 commits into from
May 17, 2016

Conversation

newzac
Copy link
Contributor

@newzac newzac commented May 16, 2016

This should resolve issue #52.

Rollback Plan

If this pull request requires anything more complex (e.g., rolling back a migration), you MUST update this section. Otherwise, delete this note.

To roll back this change, revert the merge with git revert -m 1 MERGE_SHA and perform another deploy.

URLs

QA Plan

Provide a detailed QA plan, or other developers will retain the right to mock you mercilessly.

@@ -17,6 +17,8 @@ class Git
STAGING_PREFIX = "staging"
QAREADY_PREFIX = "qaready"

RESOVLER_USED = nil

Choose a reason for hiding this comment

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

Don't use a constant for this. Make it an instance variable or something

@newzac
Copy link
Contributor Author

newzac commented May 16, 2016

:octocat: Merged into staging.2014.07.28.

1 similar comment
@newzac
Copy link
Contributor Author

newzac commented May 16, 2016

:octocat: Merged into staging.2014.07.28.

@@ -44,6 +44,10 @@ def github_repo
@github_repo || raise(MissingRequiredAttribute, "GitHub Repo is required")
end

def merge_resolver
@merge_resolver || nil
Copy link

Choose a reason for hiding this comment

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

why do we need an || nil here, if it is not defined it will just return nil

@carlallen
Copy link

QA looks good

@@ -197,6 +197,7 @@ module Octopolo
Git.should_receive(:fetch)
Git.should_receive(:perform).with("merge --no-ff origin/#{branch_name}", :ignore_non_zero => true)
Git.should_receive(:clean?) { true }
Git.should_receive(:clean?) { true }
Copy link

Choose a reason for hiding this comment

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

why is this duplicated from the line above?

Choose a reason for hiding this comment

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

expect(...).to receive(...).twice

Choose a reason for hiding this comment

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

obj.should_receive(:message).once
obj.should_receive(:message).twice
obj.should_receive(:message).exactly(3).times

obj.should_receive(:message).at_least(:once)
obj.should_receive(:message).at_least(:twice)
obj.should_receive(:message).at_least(n).times

Choose a reason for hiding this comment

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

what Carl said

@anfleene
Copy link

Let's add it to the readme too 🍻

@carlallen
Copy link

QA still good, CR good

@anfleene
Copy link

:shipit:

@newzac
Copy link
Contributor Author

newzac commented May 17, 2016

:octocat: Merged into deployable.2016.05.16.

@anfleene anfleene merged commit d7a4285 into master May 17, 2016
@emmahsax emmahsax deleted the IS-4644-springclean-automerge branch July 14, 2020 07:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants