-
Notifications
You must be signed in to change notification settings - Fork 120
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
Upgrade k8s dependencies to v0.29.3
, Fix yaml parsing issues
#907
Conversation
v0.29.3
v0.29.3
, Fix yaml parsing issues
pkg/controller/deployment_util.go
Outdated
|
||
klog.V(4).Infof("calculating proportion increase for machineSet %s. ms.desired=%d, maxDeploymentSizePossible=%d, maxDeploymentSizePossibleAsPerAnnotation=%d", is.Name, deploymentReplicas, annotatedReplicas) | ||
return integer.RoundToInt32(newISsize) - (is.Spec.Replicas) | ||
klog.V(4).Infof("calculating proportion increase for machineSet %s. ms.desired=%d, maxDeploymentSizePossible=%d, maxDeploymentSizePossibleAsPerAnnotation=%d", is.Name, newISsize, deploymentReplicas, annotatedReplicas) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should log level be 3
or 4
here ? I mean this changed log won't be visible by default. Is that desired ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed it to 3
if f == "\n" || f == "" { | ||
// ignore empty cases | ||
continue | ||
if fileAsString == "\n" || fileAsString == "" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should take note to add a unit-test to ParseK8sYaml
in the future . This is a big function and it is difficult to judge changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally we should break it down to ParseK8sCRD
, ParseMachineResources
and ParseOtherK8sResources
or something like this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
optional feedback given.
What this PR does / why we need it:
Upgrade k8s dependencies to
v0.29.3
. This is needed as we have upgraded controller-gen to v0.14.0 and it is not working withv0.28.2
of k8s dependencies. It also fixes the yaml parsing issue introduced due to updating controller-gen to0.14.0
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
IT tests pass (cc @sssash18 )
Release note: