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

validate: add weightDevice validation #570

Merged
merged 2 commits into from
Feb 12, 2018

Conversation

zhouhao3
Copy link

@zhouhao3 zhouhao3 commented Feb 6, 2018

According to the following specifications:
You MUST specify at least one of weight or leafWeight in a given entry, and MAY specify both.

implement BlkIOWeightOrLeafWeightExist.

Signed-off-by: Zhou Hao [email protected]

BlockIO: &rspec.LinuxBlockIO{
WeightDevice: []rspec.LinuxWeightDevice{
{
{
Copy link
Contributor

Choose a reason for hiding this comment

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

From Travis:

validate/validate_test.go:568:10: missing type in composite literal
validate/validate_test.go:568:10: implicit assignment of unexported field 'linuxBlockIODevice' in specs.LinuxWeightDevice literal

Copy link
Author

Choose a reason for hiding this comment

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

I don't know what to do with this, but I tried the following calls, and they all went wrong.

 WeightDevice: []rspec.LinuxWeightDevice{
        linuxBlockIODevice{
                Major: 5,
                Minor: 0,
        },
}
 WeightDevice: []rspec.LinuxWeightDevice{
        rspec.linuxBlockIODevice{
                Major: 5,
                Minor: 0,
        },
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know what to do with this, but I tried the following calls, and they all went wrong.

I'm pretty sure you want:

WeightDevice: []rspec.LinuxWeightDevice{
  rspec.LinuxWeightDevice{
    Major: 5,
    Minor: 0,
  },
},

Copy link
Author

Choose a reason for hiding this comment

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

This is still wrong.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is still wrong.

Hmm. Maybe Go has a problem with literal struct initialization for lowercase embedded types. We may need to skip these for the literal initialization and add them later via the promoted properties:

weightDevices := []rspec.LinuxWeightDevice{
  rspec.LinuxWeightDevice{},
}
weightDevices[0].Major = 5
weightDevices[0].Minor = 0

Then use WeightDevice: weightDevices when you initialize your config struct.

Copy link
Author

Choose a reason for hiding this comment

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

Cool. It worked, thanks.

errs = multierror.Append(errs,
specerror.NewError(
specerror.BlkIOWeightOrLeafWeightExist,
fmt.Errorf("You MUST specify at least one of `weight` or `leafWeight` in a given entry"),
Copy link
Contributor

Choose a reason for hiding this comment

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

This string is already covered by BlkIOWeightOrLeafWeightExist. I think we want something like:

for i, weightDevice := range r.BlockIO.WeightDevice {
    …
        fmt.Errorf("linux.resources.blockIO.weightDevice[%d] specifies neither weight nor leafWeight", i)
    …
}

Copy link
Author

Choose a reason for hiding this comment

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

updated, thanks.

@zhouhao3 zhouhao3 force-pushed the blockIO-validate branch 3 times, most recently from 09eb0a3 to 7954c88 Compare February 8, 2018 01:17
@zhouhao3
Copy link
Author

zhouhao3 commented Feb 9, 2018

@liangchenye PTAL

@zhouhao3
Copy link
Author

ping @liangchenye

@liangchenye
Copy link
Member

I'm working on implement 'kill' this morning.
I'll review it this afternoon. @q384566678

@liangchenye
Copy link
Member

liangchenye commented Feb 12, 2018

LGTM

Approved with PullApprove

@liangchenye liangchenye merged commit cac5ded into opencontainers:master Feb 12, 2018
@liangchenye liangchenye mentioned this pull request Feb 12, 2018
44 tasks
@zhouhao3 zhouhao3 deleted the blockIO-validate branch February 12, 2018 09:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants