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

Add Per-AMI Spare Instance parameter #413

Merged
merged 1 commit into from
Jan 20, 2020

Conversation

hughsaunders
Copy link
Contributor

@hughsaunders hughsaunders commented Oct 30, 2019

Currently there is a parameter for minimum number of instances for an AMI,
if you set it to n, there will always be at least n instances.

This PR adds minimum spare instances, if you set this to n, there will
always be n unsused instances waiting for new builds (unless the
instance cap is reached). This prevents new builds from having to wait
for provisioning, while also meaning that you don't need to set a high
absolute minimum.

Related: conjurinc/ops#401

@hughsaunders hughsaunders force-pushed the conjurinc-ops-401 branch 2 times, most recently from e3fb940 to 1fcdb8c Compare October 30, 2019 17:08
@res0nance res0nance added the enhancement Feature additions or enhancements label Dec 2, 2019
@hughsaunders
Copy link
Contributor Author

Hi @res0nance, thanks for your review, I've fixed the ci issues, mind having another look?
Thanks!

@res0nance res0nance requested review from jvz and thoulen January 9, 2020 01:42
Copy link
Contributor

@res0nance res0nance left a comment

Choose a reason for hiding this comment

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

Looks good to me pinging other reviewers

FYI: @erikhakansson can't request a review from you so pinging you

@erikhakansson
Copy link
Contributor

LGTM! Great idea that I'll definitely use at work.

@res0nance is that because I'm not a member of the organisation?

Copy link
Member

@jvz jvz left a comment

Choose a reason for hiding this comment

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

Code changes look alright. Could you add a test for this feature?

@hughsaunders
Copy link
Contributor Author

Code changes look alright. Could you add a test for this feature?

Will Do

Currently there is a parameter for minimum number of instances,
if you set this to n, there will always be at least n instances.

This PR adds minimum spare instances, if you set this to n, there will
always be n unsused instances waiting for new builds (unless the
instance cap is reached).

Related: conjurinc/ops#401
@hughsaunders
Copy link
Contributor Author

Added basic test

Copy link
Member

@jvz jvz left a comment

Choose a reason for hiding this comment

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

LGTM. I think we have builder classes for some of the mega-constructors you're using in the test, though I could be remembering something else.

@res0nance res0nance merged commit 701309f into jenkinsci:master Jan 20, 2020
@jetersen
Copy link
Member

jetersen commented Mar 16, 2020

Please for the love of JCasC and the next person touching this, split the data bound constructor to only be MANDATORY fields and everything else should use data bound setters.

https://issues.jenkins-ci.org/browse/JENKINS-57513
IMHO this should have never been merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Feature additions or enhancements
Projects
None yet
5 participants