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

rbd: image Open function is non-idiomatic Go #105

Closed
phlogistonjohn opened this issue Oct 31, 2019 · 3 comments · Fixed by #146
Closed

rbd: image Open function is non-idiomatic Go #105

phlogistonjohn opened this issue Oct 31, 2019 · 3 comments · Fixed by #146
Assignees

Comments

@phlogistonjohn
Copy link
Collaborator

The rbd module's Image.Open function is rather non-idiomatic for Go code. It accepts any number of interface{} arguments and then loops on them checking (only!) the type and uses those to select which ceph api function to run.
This means that x.Open("foo", true) and x.Open(true, "foo") are equivalent, as well as x.Open("foo", "foo", false, "bar", true, false, "foo", true). I've never seen go code like that! :-)

I'm not sure if this function wanted to mimic the style of the python api or what but it strikes me as odd.
It would be preferable to create more idiomatic approaches to Image type and the rbd open functions and then deprecate this approach to Open.

@sungjunyoung
Copy link

I'm not familiar about librbd :( but I think I can tell about what I think.

I'm agreed about your opinion, I'm confused about Open method's parameter when I use go-ceph first
According to the tests and sources, c_snap_name (string) and read_only (boolean) are the only required parameters for the Open method, And they are can be optional parameter. ("", false).

I think there are some options for idiomatic Go

  1. We can create two function Open, OpenReadOnly for the feature because golang doesn't support function overloading. Maybe two function have string parameter for c_snap_name
  2. Create struct for Open parameter
    • in gophercloud, there are various option struct for support openstack api parameters
    server, err := servers.Create(client, servers.CreateOpts{
      Name:      "My new server!",
      FlavorRef: "flavor_id",
      ImageRef:  "image_id",
    }).Extract()
  3. Apply more declarative parameters. like Open(readOnly bool, snapName string)

@phlogistonjohn phlogistonjohn self-assigned this Dec 30, 2019
@phlogistonjohn
Copy link
Collaborator Author

I've started working on this. PR coming soon(ish).

@phlogistonjohn
Copy link
Collaborator Author

Anyone interested in this topic please look at #146 and provide feedback. We intend to change the typical workflow such that new open functions return the rbd Image type and Create functions only return an error (like the ceph apis).

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 a pull request may close this issue.

2 participants