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

Bugfix: don't call kubectl set resources when there would be no effect. #125

Merged
merged 1 commit into from Mar 15, 2018

Conversation

Projects
None yet
3 participants
@x13n
Member

x13n commented Mar 15, 2018

Step towards fixing kubernetes/kubernetes#61190, which is k8s 1.10.0 release blocker.

@googlebot

This comment has been minimized.

googlebot commented Mar 15, 2018

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

@googlebot

This comment has been minimized.

googlebot commented Mar 15, 2018

CLAs look good, thanks!

log "Nothing to update."
return
fi
if [ ${REQUESTS_FLAG} ] || [ ${LIMITS_FLAG} ]

This comment has been minimized.

@kawych

kawych Mar 15, 2018

Member

Is it possible that code reaches this point and $REQUESTS_FLAG and/or $LIMIT_FLAG are empty?

This comment has been minimized.

@x13n

x13n Mar 15, 2018

Member

Yes, but only under quite bizarre circumstances - when all values are not set anywhere (neither in defaults, nor in scaling policy), but resources are configured in the daemonset. The code above will figure it should update something, but both flags will be empty. Shouldn't happen in practice.

@x13n

This comment has been minimized.

Member

x13n commented Mar 15, 2018

If there will be no further comments, I will merge it in 1h.

@kawych

This comment has been minimized.

Member

kawych commented Mar 15, 2018

/lgtm

@x13n x13n merged commit 15cbc4b into GoogleCloudPlatform:master Mar 15, 2018

2 checks passed

cla/google All necessary CLAs are signed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment