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

Parameterize minStartupPods #8793

Merged
merged 1 commit into from
Jun 11, 2015
Merged

Conversation

jayunit100
Copy link
Member

heres a quickie to parameterize minStartupPods ( #8666 ).

@k8s-bot
Copy link

k8s-bot commented May 26, 2015

Can one of the admins verify that this patch is reasonable to test? (reply "ok to test", or if you trust the user, reply "add to whitelist")

If this message is too spammy, please complain @ixdy.

@jayunit100
Copy link
Member Author

@mbforbes @timothysc @rrati @eparis ok heres the parameter thingy we need so you can use minStartupPods > 0

@timothysc
Copy link
Member

+1 to option enable on this param.

@ixdy ixdy added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 26, 2015
@ixdy
Copy link
Member

ixdy commented May 26, 2015

It'd probably be good to update the various platforms in hack/ginkgo-e2e.sh with reasonable defaults for this parameter, but this is fine for now.

// are found to be running and ready; it ensures that *all* pods it finds
// are running and ready. This is the minimum number it must find.
// TODO : Add command line option for this so that the number is non trivial.
minStartupPods = 0
Copy link
Contributor

Choose a reason for hiding this comment

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

Hahaha, that giant comment I wrote so that people would understand the function. No more!

Copy link
Member

Choose a reason for hiding this comment

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

yeah... unfortunately the comment already didn't match the line following it.

maybe everything after "More verbosely" should be moved to the comment for waitForPodsRunningReady.

@thockin
Copy link
Member

thockin commented May 27, 2015

Please triage wrt code freeze

@jayunit100
Copy link
Member Author

just to confirm : are we triaging e2e updates ?

@thockin
Copy link
Member

thockin commented May 27, 2015

Sorry, you're right, I was too quick.

Needs rebase, though

@jayunit100
Copy link
Member Author

ah yes, ok ill rebase against some other pending prs also

@thockin
Copy link
Member

thockin commented May 27, 2015

sorry, still needs rebase

@mbforbes
Copy link
Contributor

I'm out yesterday/today/tomorrow—@thockin, @ixdy, either of you willing to steal this review? Sorry / thanks!

@thockin
Copy link
Member

thockin commented May 28, 2015

I am on-call, no can do today

@thockin
Copy link
Member

thockin commented May 28, 2015

removing LGTM

@thockin thockin removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 28, 2015
@k8s-bot
Copy link

k8s-bot commented Jun 10, 2015

Can one of the admins verify that this patch is reasonable to test? (reply "ok to test", or if you trust the user, reply "add to whitelist")

If this message is too spammy, please complain to ixdy.

@jayunit100
Copy link
Member Author

ok finally rebased ....... ready to look at again

@mbforbes mbforbes added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 11, 2015
@mbforbes
Copy link
Contributor

LGTM, thanks

ArtfulCoder added a commit that referenced this pull request Jun 11, 2015
Parameterize minStartupPods
@ArtfulCoder ArtfulCoder merged commit bd6db7b into kubernetes:master Jun 11, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants