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

Flag commits scheduled to next deploy #887

Merged
merged 1 commit into from Apr 12, 2019

Conversation

Projects
None yet
5 participants
@esnunes
Copy link
Contributor

esnunes commented Apr 4, 2019

Problem

In a stack with continuous deployment enabled, by looking at the list of Undeployed Commits it is not clear which of them are going to be grouped and deployed.

Solution

Based on the config value deploy.max_commits flag the commits that are supposed to be grouped and deployed.

Notes

This PR was created on top of #890 .

Screenshot 2019-04-05 at 15 02 36

@esnunes esnunes self-assigned this Apr 4, 2019

@esnunes esnunes requested a review from JackTLi Apr 4, 2019

@esnunes esnunes changed the base branch from master to currently-deploying Apr 5, 2019

@esnunes esnunes force-pushed the next_deploy branch 2 times, most recently from 4646dfd to de41ebb Apr 5, 2019

@esnunes esnunes requested review from DazWorrall and casperisfine Apr 5, 2019

@esnunes esnunes added the enhancement label Apr 5, 2019

@casperisfine
Copy link
Contributor

casperisfine left a comment

Implementation looks good to me besides a couple nitpicks on naming.

As for the feature I honestly don't have much opinion on it.

@@ -35,12 +35,30 @@ def deploy_disallowed?
end

def deploy_discouraged?
stack.maximum_commits_per_deploy && index >= stack.maximum_commits_per_deploy
hit_maximum_commits_per_deploy?

This comment has been minimized.

Copy link
@casperisfine

casperisfine Apr 5, 2019

Contributor

It's weird to extract the method content, to finally just alias it.

This comment has been minimized.

Copy link
@esnunes

esnunes Apr 5, 2019

Author Contributor

I've extracted the method content to re-use it in the deploy_scheduled? predicate. In the context of deploy_scheduled?, the method name deploy_discouraged? does not express the conditions needed to validate the proposition.


private

def hit_maximum_commits_per_deploy?

This comment has been minimized.

Copy link
@casperisfine

casperisfine Apr 5, 2019

Contributor

Not a huge fan of that naming. I'm not a good english speaker, but somehow maximum_commits_per_deploy_hit? or even maximum_commits_per_deploy_reached? would feel more natural to me.

However I defer to native speakers.

This comment has been minimized.

Copy link
@esnunes

esnunes Apr 5, 2019

Author Contributor

I agree with you, I wasn't happy with the naming too, thanks!

This comment has been minimized.

Copy link
@JackTLi

JackTLi Apr 5, 2019

Member

I like maximum_commits_per_deploy_reached? 🙂

@esnunes esnunes force-pushed the next_deploy branch from de41ebb to 66969cb Apr 5, 2019

@DazWorrall
Copy link
Member

DazWorrall left a comment

Couple of nits but looks good 🎉

Show resolved Hide resolved app/views/shipit/commits/_commit.html.erb Outdated
Show resolved Hide resolved test/models/undeployed_commits_test.rb Outdated
Show resolved Hide resolved test/models/undeployed_commits_test.rb Outdated

@esnunes esnunes force-pushed the next_deploy branch from 66969cb to c9bdfc5 Apr 5, 2019

@JackTLi

JackTLi approved these changes Apr 5, 2019

@doodzik

doodzik approved these changes Apr 8, 2019

@esnunes esnunes force-pushed the next_deploy branch from c9bdfc5 to f5f83bc Apr 12, 2019

@esnunes esnunes force-pushed the next_deploy branch from f5f83bc to c3d7b61 Apr 12, 2019

@esnunes esnunes merged commit d9f7a8a into currently-deploying Apr 12, 2019

0 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/travis-ci/push The Travis CI build failed
Details

@esnunes esnunes deleted the next_deploy branch Apr 12, 2019

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.