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.

Sign up for GitHub

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

Jump to bottom

Fill kubemacpool range in the fillDefaults function #99

Merged
merged 1 commit into from May 15, 2019
Merged

Fill kubemacpool range in the fillDefaults function #99

merged 1 commit into from May 15, 2019

Conversation

SchSeba
Copy link
Contributor

@SchSeba SchSeba commented May 14, 2019 •
edited

This PR moves the range generation from the render to the fillDefaults functions.

This change is Reviewable

@SchSeba SchSeba requested a review from phoracek May 14, 2019 13:13
phoracek
phoracek requested changes May 14, 2019
View reviewed changes
Copy link
Member

@phoracek phoracek left a comment

Choose a reason for hiding this comment

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

Please buff the commit message. Explain why do we change it.

pkg/network/image-pull-policy.go Outdated
@@ -21,14 +21,16 @@ func validateImagePullPolicy(conf *opv1alpha1.NetworkAddonsConfigSpec) []error {
return []error{}
}

func fillDefaultsImagePullPolicy(conf, previous *opv1alpha1.NetworkAddonsConfigSpec) {
func fillDefaultsImagePullPolicy(conf, previous *opv1alpha1.NetworkAddonsConfigSpec) error {
Copy link
Member

Choose a reason for hiding this comment

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

We should use []error instead like we do in validate functions. So in case multiple components have invalid config, we report all at once.

pkg/network/kubemacpool.go Outdated
@@ -84,6 +71,28 @@ func renderKubeMacPool(conf *opv1alpha1.NetworkAddonsConfigSpec, manifestDir str
return objs, nil
}

func fillDefaultsKubeMacPool(conf, previous *opv1alpha1.NetworkAddonsConfigSpec) error {
Copy link
Member

Choose a reason for hiding this comment

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

please move this below validateKubeMacPool, so it follows the execution order

pkg/network/network.go Outdated
if err != nil {
return err
}
err = fillDefaultsKubeMacPool(conf, previous)
Copy link
Member

Choose a reason for hiding this comment

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

nit: empty line before this row

pkg/network/network.go Outdated
func FillDefaults(conf, previous *opv1alpha1.NetworkAddonsConfigSpec) {
fillDefaultsImagePullPolicy(conf, previous)
func FillDefaults(conf, previous *opv1alpha1.NetworkAddonsConfigSpec) error {
err := fillDefaultsImagePullPolicy(conf, previous)
Copy link
Member

Choose a reason for hiding this comment

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

like in Validate, please collect all errors and return only in the end

pkg/network/kubemacpool.go Outdated
@@ -84,6 +71,28 @@ func renderKubeMacPool(conf *opv1alpha1.NetworkAddonsConfigSpec, manifestDir str
return objs, nil
}

func fillDefaultsKubeMacPool(conf, previous *opv1alpha1.NetworkAddonsConfigSpec) error {
if conf.KubeMacPool.RangeStart == "" || conf.KubeMacPool.RangeEnd == "" {
if previous != nil && previous.KubeMacPool != nil {
Copy link
Member

Choose a reason for hiding this comment

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

What if kubeMacPool: {}. Is it considered nil or empty structure?

pkg/network/kubemacpool.go Outdated
@@ -84,6 +71,28 @@ func renderKubeMacPool(conf *opv1alpha1.NetworkAddonsConfigSpec, manifestDir str
return objs, nil
}

func fillDefaultsKubeMacPool(conf, previous *opv1alpha1.NetworkAddonsConfigSpec) error {
if conf.KubeMacPool.RangeStart == "" || conf.KubeMacPool.RangeEnd == "" {
Copy link
Member

Choose a reason for hiding this comment

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

please add an inline comment saying that if user hasn't explicitly requested a range, we try to reuse previously applied

pkg/network/kubemacpool.go Outdated
return nil
}

prefix, err := generateRandomMacPrefix()
Copy link
Member

Choose a reason for hiding this comment

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

please add an inline comment that if no range was specified, we generated a random prefix

@SchSeba
Copy link
Contributor Author

SchSeba commented May 14, 2019

@phoracek can you please make another review

phoracek
phoracek previously approved these changes May 14, 2019
View reviewed changes
Copy link
Member

@phoracek phoracek left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks :)

pkg/network/image-pull-policy.go Outdated
if conf.ImagePullPolicy == "" {
if previous != nil && previous.ImagePullPolicy != "" {
conf.ImagePullPolicy = previous.ImagePullPolicy
} else {
conf.ImagePullPolicy = defaultImagePullPolicy
}
}

return nil
Copy link
Member

Choose a reason for hiding this comment

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

this needs to be []error{}, this would fail (right?)

pkg/network/kubemacpool.go Outdated
if prev.KubeMacPool != nil && !reflect.DeepEqual(prev.KubeMacPool, next.KubeMacPool) {
return []error{errors.Errorf("cannot modify KubeMacPool configuration once it is deployed")}
}
return nil
Copy link
Member

Choose a reason for hiding this comment

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

Oh I see it was used before. Wonder how that works with append

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep want me to change it to be on the safe side?

Copy link
Member

@phoracek phoracek May 14, 2019 •
edited

Choose a reason for hiding this comment

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

Oh yes, we should, otherwise we endup with []error{<nil>}. If you don't mind

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done :) can you please take a look?

@SchSeba
Fill kubemacpool range in the fillDefaults function
d4b64ce 
This PR moves the range generation from the render to the fillDefaults functions.

We need this PR so we are able to apply again the cluster CR.

Before this PR we get an error on the operator that update is not
supported on the kubemacpool because we generate a new range and then
the CR is different from the applied one.
phoracek
phoracek approved these changes May 15, 2019
View reviewed changes
Copy link
Member

@phoracek phoracek left a comment

Choose a reason for hiding this comment

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

I think there is a minor bug, but not introduced in this PR. LGTM

pkg/controller/networkaddonsconfig/networkaddonsconfig_controller.go
network.FillDefaults(&networkAddonsConfig.Spec, prev)
if err := network.FillDefaults(&networkAddonsConfig.Spec, prev); err != nil {
log.Printf("failed to fill defaults: %v", err)
errors.Wrapf(err, "failed to fill defaults: %v", err)
Copy link
Member

Choose a reason for hiding this comment

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

looks like a bug, we don't use the return value. i don't think that err is updated in place

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes you are right, Can I create a new PR to fix this?

@SchSeba SchSeba merged commit aeb5d68 into kubevirt:master May 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@phoracek phoracek phoracek approved these changes

Assignees
No one assigned
Labels
size/M
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@SchSeba @phoracek @kubevirt-bot