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
operator: make CephBlockPoolRadosNamespace and CephFilesystemSubVolumeGroup get outputs more verbose #14049
Conversation
f545682
to
c753b13
Compare
Did assume that Distributed was the default based on PinCephFSSubVolumeGroup function, with this information I also did slight change to validatePinningValues as they were a bit contradicting, let me know in case there was some misunderstanding here on my part. Other than that I was not exactly sure if pinningType, pinningConfig or pinningSetting would best describe the field. |
c753b13
to
b60f1df
Compare
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.
just some small questions and naming suggestions
if !(*pinning.Distributed == 1) && !(*pinning.Distributed == 0) { | ||
err = errors.Errorf("validate pinning type failed, Distributed: unknown value %d", *pinning.Distributed) | ||
if *pinning.Distributed != 0 && *pinning.Distributed != 1 { | ||
return fmt.Errorf("validate pinning type failed, Distributed: value must be 0 or 1, got %d", *pinning.Distributed) |
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.
The changes look equivalent to me in this method. @parth-gr Any concerns?
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.
I think no need for the change unless it improves anything,
And with the updated change,
we look for this only if the config is correct, but this is the first priority to be check.
if numNils > 1 {
return fmt.Errorf("only one can be set")
}
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.
Okay, I see so earlier returning is disabled by choice as this error should have priority makes sense, thanks for explanation @parth-gr! Okay, then I will just adjust the error message that has the 1.1 instead of 1.0
b60f1df
to
1e65dec
Compare
This pull request has merge conflicts that must be resolved before it can be merged. @NymanRobin please rebase it. https://rook.io/docs/rook/latest/Contributing/development-flow/#updating-your-fork |
Thanks for the comments @travisn was a bit unsure how to name the pinning field but let's go with just pinning :) |
1e65dec
to
8aa89dd
Compare
ace1384
to
aed6a1f
Compare
The following printcolumns were added CephBlockPoolRadosNamespace: Phase, BlockPoolName, Age CephFilesystemSubVolumeGroup: Phase, FilesystemName, PinningConfig, Age Also improved the error messages in SubVolumeGroupClient Signed-off-by: NymanRobin <nyman.robin@gmail.com>
aed6a1f
to
70a7e61
Compare
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.
lgtm
operator: make CephBlockPoolRadosNamespace and CephFilesystemSubVolumeGroup get outputs more verbose (backport #14049)
The following printcolumns were added
CephBlockPoolRadosNamespace: Phase, BlockPool, Age
CephFilesystemSubVolumeGroup: Filesystem, Quota, Pinning
Related to issue: #13775