Discussion: Project structure/design
Tinyblargon opened this issue · 9 comments
Currently there is this huge package called proxmox and this huge Client class. I'm proposing at some point in the future we move the project to a new structure/design. As this project matures some things will have to change which will end up breaking peoples code. In my opinion the sooner we decide what kind of action to take the less people we will possibly upset with breaking changes.
#Proposal 1
Would it be an idea to move toward a structure where the project is a bit more split up?
So instead of having this one package called proxmox we have lots of smaller packages. Packages like qemu lxc acmeplugin acmeaccount client metrics storage user util node session validate
Example:
Currently there is a proxmox.ConfigLxc and proxmox.ConfigQemu struct.
Wouldn't it be more logical if these would be lxc.config and qemu.config
#Proposal 2
Would it be an idea to reduce the size of the Client class and move that into the qemu lxc acmeplugin acmeaccount metrics storage user node files? as currently a lot the functions in this class are only referenced once and only add a url and pass on the params map[string]interface{} variable.
Example:
this function would be moved into pool.go
func (c *Client) UpdatePoolComment(poolid string, comment string) error {
return c.UpdateItem(map[string]interface{}{
"poolid": poolid,
"comment": comment,
}, "/pools/"+poolid)
}And would be written as the following:
func UpdatePoolComment(c *Client , poolid string, comment string) error {
return c.UpdateItem(map[string]interface{}{
"poolid": poolid,
"comment": comment,
}, "/pools/"+poolid)
}#Conclusion
I understand that making these kind of changes would drastically change the look of the project.
But there is a benefit to these changes. It would give new devs and users of the project a better idea of where to find things/put new things.
By splitting up the project the auto-completion/proposals of the ide get more usable. For example: the Client class has over 120 functions which makes it difficult to use if your new to the project, and can be quite daunting if your importing this as a library.
Currently only the LXC, Qemu and pool related functions have been used in the terraform provider. Which means moving all the other code to the new style would be relatively quick. The LXC, Qemu and pool could be moved slowly with small parts at the time as to not break compatibility with the terraform provider.
#Feedback
I am wondering how my fellow contributors feel about these proposed changes.
This seems like it may be a good idea. I am new to contributing to this project so changes like these wouldn't affect my code that much. However my newness can speak to how easy / difficult it is for newbies coming along to the project. To me there does seem to be a bit of duplication of concern across the individual files and the client file. Moving to either of your proposals would help to cut down on potential duplicate code and improve the ease of adding new parts of the Proxmox API to it. Sort of like making a standardised API in the client.go file which all the individual modules can use.
A benefit to this that stands out to me would be allowing the users of this project to see the error messages the API sends back. In my network bit I have added some methods (which at first glance look redundant due to the current architecture and me not wanting to break other people's code) that will return the body of the API response which contains useful error messages for users of this program and also the terraform and packer providers. The current error messages which are solely the HTTP errors aren't always helpful in working out why a certain config isn't working. I struggled with this when using the terraform proivder and actually commited an update to the documentation when I worked out what the issue was.
I would be very willing and keen to help with this refactoring effort and with documenting a lot of the codes base (to keep go lint happy). Even if you want to create a branch and we collab on it a bit to work out a proof of concept?
In my opinion, we should definitely go down the path of the #2 proposal.
Client.go should only contain the code common to the whole project, Session.go should disappear as useless (in my opinion) and every configuration object in the API should have all its methods inside
The problems with the proxmox provider are absolutely manageable, the important thing in my opinion is to have some code that fully respects the API (see the disks part which is completely flawed in the current approach). Starting from this basis, everything should then be feasible.
Another important point are the tests. Before CircleCI was deprecated, setting up a debian instance to install proxmox and running the code was fairly straightforward. Now, github actions do not support nested virtualization and it is not possible in any way to test the code except through API calls to some external instance, we should also solve this point.
I've just started doing a bit of an investigation into doing this. What are people's thoughts on how the various functions in the separate files should be accessed from the wider project? I've noticed a lot of the functions are receiver functions receiving the a client pointer. In practice this means these functions are only accessible as a dot call from an object with the client type. Should this still be the case or should each function be accessible everywhere in the package? And potentially accessible from outside the package?
@mleone87 why do you think Session.go is useless? Just so I can get an understanding of your point of view on it. Thanks
I have just had a look at this project due to it being the backing for the (Terraform plugin)[https://github.com/Telmate/terraform-provider-proxmox], and wanting to look at potentially implementing some more of the api in here.
As a newbie coming to the project and wanting to implement some more features, I definitely agree with the idea of splitting it up as has been suggested. I think tests are definitely a strong part of making sure that changes don't break the projects that rely on this one.
I know there's a certain amount of testing that has been written, and I've done some tweaks to the Vagrant config to get it running on my mac (I'll test it on Windows and Linux too, before raising a pull request). Does anyone (who knows the project better than I) know whether the tests cover all that is used by the terraform provider for example?
@rf152 Currently pretty much none of the code that has been used by the terraform provider has sufficient testing. Currently I'm in the process of improving that with #187. While also moving more logic into this project so implementations like vagrant and terraform can be less complex.
A lot of work will have to be done before everything is covered by proper test
Ok, so I guess part of the work that could be done is improving the tests (which may well be something I'm able to help with).
I'll start to have a look to see what I can write tests for.
Ok, so I've now got tests covering the stuff that the Terraform provider uses. The only bit that's not properly tested is the cloud-init stuff (which doesn't work, in my experience - hence why I originally came to look at this repo!)
Is it worth completely rewriting the Client class, or is the plan to slowly remove bits from it until it's just the core stuff that is central to the whole project?
I've done a partial rewrite of the client class. So far it only uses API tokens for authentication but it allows for http requests to the proxmox api and returns, in my opinion, more useful errors when the API returns an error. It also doesn't allow for custom HTTP headers to be set because I was unsure of the benefit of this. Feature's can be added to bring it up to feature parody with the current client class if more features are needed. I've been trying to keep it as simple as possible to prevent adding to much and creating a lot of technical debt.
My rewritten client file can be found in an orphan branch on my fork of this project here