go-ceph
go-ceph copied to clipboard
ceph
nfs admin: add SecType field
Ceph is adding a flag to get/set NFS-Ganesha's SecType configuration opiton. This can be used to specify authentication types for an export. These changes only affect struct fields and are fully backwards compatible with existing code. Therefore no API status flags are added to the code. Test cases added - with api buildtag for ceph main branch.
Fixes: #764
Checklist
- [x] Added tests for features and functional changes
- [x] Public functions and types are documented
- [x] Standard formatting is applied to Go code
- [x] Is this a new API? Added a new file that begins with
//go:build ceph_preview<-- see description
The corresponding changes have not been commited to ceph yet. Tests are expected to fail until https://github.com/ceph/ceph/pull/47934 is merged.
@ansiwen @anoopcs9 how do you feel about having only the tests behind the build tag? The code is all changes to existing structs and is backwards compatible with older mgrs if you don't set sectype. I'm ok with this approach if you are. But if no, there are two OKish approaches I see: (a) put the actual sectype values "enum" into a separate file with the build tag or (b) see if we can make sectype private and only allow setting it via a method. Personally, I don't like (b) because it changes the workflow - you can set most of the fields directly, but not others.
I'd like to get this one merged before the next release so if you could provide feedback on this soon, I'd really appreciate it.
@ansiwen @anoopcs9 how do you feel about having only the tests behind the build tag? The code is all changes to existing structs and is backwards compatible with older mgrs if you don't set sectype. I'm ok with this approach if you are.
Git history tells me that we have had similar changes in the past and I am totally in favour of current approach.
@ansiwen @anoopcs9 how do you feel about having only the tests behind the build tag? The code is all changes to existing structs and is backwards compatible with older mgrs if you don't set sectype.
So, we are actually talking about two different tags, right? I don't have a problem with setting the build constraint ceph_main only on the tests.
The purpose of the ceph_preview tag however is to give you the possibility to change new exported APIs after release, in case you discover shortcomings in the field. If you are confident, that you don't need that in this case, I'm fine with skipping it. Otherwise you could move the affected code into two mutually exclusive files, one with the current and one with the preview version.
Right, it should probably have preview in addiiton to ceph_main where ceph_main is present. Splitting the code up is (I think) not worth it because the changes are a new type (aliased string), some constants for that type and an addition to existing structs. The only way to really split the code would be more hassle (for the users, I think). So I think I'll fix the tagging in test.go file and leave the code as it is.
Oh.. PR is still in Draft state? I take the liberty to mark it as ready.
Oops, thanks. I had left it in draft for deciding on the code tagging but meant to change ti for ready-for-review yesterday after I updated the test file.
Right, it should probably have preview in addiiton to ceph_main where ceph_main is present.
Hmm, I can't follow that thought. ceph_preview is meant to mask out exported identifiers, so you only need to add it to tests if these tests rely on these. Since you decided to not mask the new exported stuff, you wouldn't need to do it for the tests either.
Splitting the code up is (I think) not worth it because the changes are a new type (aliased string), some constants for that type and an addition to existing structs. The only way to really split the code would be more hassle (for the users, I think). So I think I'll fix the tagging in test.go file and leave the code as it is.
I'm not objecting your decision, I agree it's a small impact on the exported API, and if you are confident that you won't need to revise, I'm fine. However, I don't understand the "for the users" part. How does it make a difference for the user? It should not matter for the user in which files the exported identifiers are defined.
In any case I'm going to approve. If you decide to merge as is, just remove the do-not-merge tag.
Regarding making things harder for a user of the API - I had suggested two ways of putting some of the code behind a build tag: (a) was to move the constants to a separate file, but (b) was to add a method like SetSecType and hide sectype field as private. I was mostly referring to (b) when I wrote that doing extra work to put the option behind a build tag was possibly going to be annoying for the users. The current workflow is create a struct with the settings you want and call a method with that struct as an argument. Option (b) means that you can directly set most fields, but then need a special setter method for one field. It wouldn't be the worst thing in the world, but I didn't feel the risk of people trying to set SecType on a version of ceph that doesn't have it was worth that altered workflow.
Does that explain my earlier comments well enough?
Does that explain my earlier comments well enough?
Yes, thanks. I was confused, because what I would have done has no impact for the users, and I thought you are referring to that. (Which is: moving all exported identifiers, that you want to change, to a file foo_old.go guarded with !ceph_preview and coping and changing it into another file foo.go guarded with ceph_preview. When it changes to stable we would remove the build constraint in foo.go and delete the foo_old.go file.)