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

planner: add functions to check if smbshares are compatible #260

Merged

Conversation

phlogistonjohn
Copy link
Collaborator

Compatible SmbShares are shares that can be hosted by one smbd instance.
In other words, compatible share can be grouped together. This function
is not yet used but will be once we have the ability to specify grouped
smbshares.

Adds a newer version of testify and matching unit tests.

To use the the ErrorContains function.

Signed-off-by: John Mulligan <[email protected]>
Compatible SmbShares are shares that can be hosted by one smbd instance.
In other words, compatible share can be grouped together. This function
is not yet used but will be once we have the ability to specify grouped
smbshares.

Signed-off-by: John Mulligan <[email protected]>
Copy link
Collaborator

@synarete synarete left a comment

Choose a reason for hiding this comment

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

LGTM.
Minor comment on pass-by-value vs pass-by-pointer.

}

func incompatible(
current, existing InstanceConfiguration,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I prefer using pass-by-pointer in such cases. Although InstanceConfiguration is just a 4-pointers tuple now, it may get bigger in the future causing inefficient pass-by-value.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is a case where I personally feel semantics > performance UNTIL we profile and discover this is leading to a measurable performance issue. I want the "read only" nature of this comparison to be apparent.

On top of that https://codewithyury.com/golang-pass-by-pointer-vs-pass-by-value/ notes that pass by value is not always slower. I one read a longer and more thorough article saying the same but I can't find it right now.

// CheckCompatible returns an error if the instance configurations are
// not compatible with each other. A compatible instance is one that
// can share the same smbd.
func CheckCompatible(current, existing InstanceConfiguration) error {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as previous comment: prefer pass-by-pointer.

Copy link
Collaborator

@anoopcs9 anoopcs9 left a comment

Choose a reason for hiding this comment

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

lgtm

@phlogistonjohn phlogistonjohn enabled auto-merge (rebase) October 6, 2022 18:19
@phlogistonjohn phlogistonjohn merged commit 1261fad into samba-in-kubernetes:master Oct 6, 2022
@phlogistonjohn phlogistonjohn deleted the jjm-compatible branch October 20, 2022 17:57
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