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

Aiming for v2? #14

Open
ernierasta opened this issue Jul 11, 2019 · 7 comments
Open

Aiming for v2? #14

ernierasta opened this issue Jul 11, 2019 · 7 comments

Comments

@ernierasta
Copy link
Contributor

Hi @iamacarpet!

Yesterday I was looking at go-win64api API, and it may be good to think about current state. :-) I would like to introduce API revamp and a lot of braking changes. I think go modules (or generally releasing versions) could help with compatibility. I have no experiences with that so far, but maybe You have, or I will figure out how to do this correctly if You would be interested in doing this.

If You consider aiming for v2 of this library I would like to see following changes:

  • IDE autocomplete and user memory friendliness:
    All functions should start with topic name so for example:
    ListLocalGroups would become LocalGroupsList (or GroupList)
    InstalledSoftwareList would become SoftwareListInstalled
  • Better function names in general (very subjective):
    • IsLocalUserAdmin would be UserIsLocalAdmin
    • IsDomainUserAdmin would be UserIsDomainAdmin
    • AddGroupMembership would be GroupAdd
    • ChangePassword would be UserPassword
  • Other changes which could make API nicer. F. e. should LocalGroup* become just Group*. It is unlikely we will be manipulating domain groups, so maybe words Local vs Domain should be used only when we have functions to work with both?
  • Maybe think about function parameters and returned values? At first glance they are ok IMO.
  • Better godoc documentation - good package description (most of current readme) ,more comments, move examples to documentation, add godoc badge to readme, shorten readme.

This would make v2 completely incompatible with version 1, but maybe with easier to use and cleaner API?

I do not want to force this change if You feel it would not be good for this library, and will completely understand if You do not wont to go this way. It is just idea and I may be wrong, but if You like this I am here to help and I can do most of mentioned changes, I would only need critical eye to watch me carefully. ;-)

@iamacarpet
Copy link
Owner

Hello again @ernierasta ,

All of your suggestions sounds pretty great, so would be very happy to work collaboratively on some PRs to head in this direction - I'll try and provide as much oversight and help as possible, but I can't promise I'll have a lot of time to dedicate to it at the moment.

I think go modules (or generally releasing versions) could help with compatibility.

Yes, Go modules definitely sounds like the direction of travel at the moment, so this is probably best, but like you, I haven't been able fully understand the consequences of version numbering and switching to a tagged release format when one wasn't used before, especially if everything using it currently is just doing a basic "go get" pull into their projects.

I can say that looking at the traffic graphs that GitHub provides, there are apparently over 100 pulls a day from a small number of unique users, which suggests that someone or some people are using it in a CI/CD pipeline - so we'd probably better figure out how not the break that before merging into master, or changing the default branch.

Perhaps make all the changes in a "v2" branch first, then worry about any migration of users later, when we've got something to present to them?

Regards,
iamacarpet

@ernierasta
Copy link
Contributor Author

Great,
definitely, we should make separate branch. We have to do it in way, we will not break anyone's code.

Ok, so I will work now on my firewall PR, then I will look into this.

O.T: I think there may be quite a lot of users, but they are not as active (in sense of issues and PR's) because a) they are happy how library works, but also b) they are often from "proprietary, closed source world", so they have different mind set. :-)

@creativeprojects
Copy link

One way to do it would be to keep the current API as it is, for compatibility.
Just mark them as deprecated in the description.
This way it doesn't break any existing code.
Any new development would take the new and documented methods 😛

@iamacarpet
Copy link
Owner

@ernierasta just re-read your suggestions and a lot of them are really good, thanks again!

I might have another internal project involving this library, so may be able to find an excuse to put in the time required to push out a v2

Go’s module support is more mature now, so doing a v2 in another branch and using Git tags / GitHub releases is more feasible now.

I think rather than just renaming functions, I might look to create folders with groups of functions in, so you can import just the pieces you want & it’ll make naming easier (as the function name doesn’t have to contain everything, half of it will be in the import path itself): what do you all think about that?

I’ll also look to merge in some changes from the forks, if their contributors don’t mind.

@jjcabrera-terainsights your repo stands out as having the most changes, would you be ok with me merging in some of your changes upstream? And do you have any comments about their stability, or if you’ve got any documentation to bundle?

@ernierasta
Copy link
Contributor Author

Hi Sam!

Separating functions to separate packages is good idea. The only downside is - more imports, but usually only one/two functions are needed and many times they probably would be from the same package. More granular imports means importing less code you do not use, so generally better maintainability from user perspective. And would cleanup API nicely.

iamacarpet added a commit that referenced this issue Sep 1, 2020
iamacarpet added a commit that referenced this issue Sep 1, 2020
still need to move group functions into identity/group

also need to move shared definitions into an internal package, ideally grouped by Windows header file
iamacarpet added a commit that referenced this issue Sep 1, 2020
includes initial draft of splitting definitions into their own internal packages

not sure if the definitions should be moved to match their header file location in the Windows SDK, for better grouping
@iamacarpet
Copy link
Owner

Well, that's all I have time for today - not sure when I'll be on this again.

In the meantime, request for comments - does this fit with what you guys were expecting so far?

@ernierasta you'll probably have seen the PR open for some firewall changes I saw you'd got in your fork.

If they are still relevant, don't suppose you fancy updating them for v2-develop and opening a new PR there?

@icanhazpython
Copy link

icanhazpython commented Jun 27, 2022

I was hoping to use some of the file permissions/ACL stuff, but looks like that is in the v2 branch. Also looks like there were other changes made to the master, so the v2 branch would need to have those merged in to make it current. Am I reading that correctly? I'm attempting to run the v2 branch but getting the following undefined's - looks like they are present in master though:

../../../go/pkg/mod/github.com/iamacarpet/go-win64api/[email protected]/permissions.go:288:18: undefined: GetRawSidForAccountName
../../../go/pkg/mod/github.com/iamacarpet/go-win64api/[email protected]/permissions.go:293:18: undefined: ConvertRawSidToStringSid
../../../go/pkg/mod/github.com/iamacarpet/go-win64api/[email protected]/profile.go:87:17: undefined: GetRawSidForAccountName
../../../go/pkg/mod/github.com/iamacarpet/go-win64api/[email protected]/profile.go:92:14: undefined: ConvertRawSidToStringSid
../../../go/pkg/mod/github.com/iamacarpet/go-win64api/[email protected]/sessions.go:102:27: undefined: IsLocalUserAdmin
../../../go/pkg/mod/github.com/iamacarpet/go-win64api/[email protected]/sessions.go:106:27: undefined: IsDomainUserAdmin
../../../go/pkg/mod/github.com/iamacarpet/go-win64api/[email protected]/sysinfo.go:563:38: undefined: GetBitLockerRecoveryInfoForDrive

I'm curious if it's possible to run v2 as-is in any way, and if not, curious about what would be required to get this thing fully up to date. I think the master branch calls for go v1.13, and I am currently on v1.18, so seems like some updates are in order.

I went through all of the forks for this project and did identify one that has implemented the file permissions stuff from v2:

Here are some other forks with significant changes:

Not sure if any of those would be interested in submitting PRs for their work, but could be something to look into more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants