Add support for rotating/revoking currently authenticated personal access token

[theipster]

Relates to #1686.

This PR does the following:

1. Adds RotatePersonalAccessTokenSelf for invoking the POST /personal_access_tokens/self/rotate endpoint;
2. Adds RevokePersonalAccessTokenSelf for invoking the DELETE /personal_access_tokens/self endpoint;
3. Renames the ambiguous RotatePersonalAccessToken to RotatePersonalAccessTokenByID;
4. Renames the ambiguous RevokePersonalAccessToken to RevokePersonalAccessTokenByID;
5. Adds a backwards-compatibility shim RotatePersonalAccessToken that calls RotatePersonalAccessTokenByID; and,
6. Adds a backwards-compatibility shim RevokePersonalAccessToken that calls RevokePersonalAccessTokenByID.

[svanharmelen]

I guess I'm OK with this one as it seems to reflect the docs just a bit better. Yet the same thing about tests applies here as well of course...

[theipster]

@svanharmelen The new Test*PersonalAccessTokenByID functions introduced here are based off the corresponding existing Test*PersonalAccessToken functions, which also don't have any error handling around those static time.Parse() calls.

If that gets updated upstream, then I'm happy to update this PR to follow suit.

Indeed, if you are happy to suggest an error message (I can't think of an appropriate one), then I'm happy to add it.

[theipster]

@PatrickRice-KSC Would you anticipate any significant impact to the gitlabhq/gitlab Terraform provider by introducing a breaking change as proposed in this PR?

[RicePatrick]

@theipster - There shouldn't be any breaking changes to the TF provider since we haven't been using the rotation API for personal access tokens.

From a pure semantic versioning perspective though, I wonder if RotatePersonalAccessToken should be left alone, and a slightly different naming convention used for rotating without the ID. I'd be worried for other consumers of the library (since this is a very popular library) consuming a breaking change on a minor version :-/

@svanharmelen owns the final call though :)

[rumenvasilev]

In an interest of keeping the API "unchanged" (no breaking change), do you mind including the *ByID and *Self and wrapping *ByID within the existing method? Then we have all the options and a minor/patch version of the SDK.

[theipster]

@rumenvasilev Not a bad idea. 💡

I've now done that and updated the PR description to reflect the overall change.

[svanharmelen]

While I fully understand the idea, I'm not sure if I like this approach. I prefer to have the package follow the docs as close as possible and not introduce these kind of bridges.

The main reason for it is that the GitLab API itself also changes a lot and once we start (trying) to work around it like this, then where is the end? So my "fear" is a lot of overhead and twisty corners that will only extend and grow over time.

The costs of not using a bridge/shim is of course that people would have to make an update to their code base, but (as annoying as that may be) in reality these kind of changes take at most a minute or two. So yes it is a breaking change, but the fix is very obvious straight forward.

Especially since the function signature will change, so it will be clear something changed and your code will not compile without making a tweak. So you will not be surprised in that you are silently calling a different API without realising it, as that would definitely be a no-go.

Thoughts?

[rumenvasilev]

I see what you’re saying. But having to keep up with multiple breaking changes would force me to implement my own funcs and methods and avoid using the majority, if not the entire sdk. As you’ve said it yourself. A small change. But as you know, those quickly add up over time.

Ultimately it’s down to the owner of the sdk to decide if they want to or not to do a breaking change.

Here’s what golang did https://github.com/golang/go/blob/45446c867a3ffdf893bdfd1e1ef9e30166eaa157/src/io/ioutil/ioutil.go#L26

[theipster]

In terms of these new functions, naming them *ByID and *Self seems a) fairly self-explanatory to me, and also b) uses the same wording as the GitLab docs, so I don't understand the concern around them not following the docs.

If the concern is not focused on these new functions but instead on the shims, then I would say it's important to remember that the shims are only intended to live for a transition period (that you retain control of) and the problem of alignment with the upstream API / docs will fix itself when the shims eventually get deleted.

Now, the question of whether you choose to delete those shims sooner or later will only really impact your users' migration path / user experience. Indeed, you could choose to never let the shims see the light of day; it'll only be your users that will feel the consequences (i.e. being forced to update their code). If you feel that's an acceptable experience for your users, then so be it. At the other extreme, you could choose to allow the shims to live in the codebase forever, and allow your users to be blissfully unaware of what's happening underneath. There's technically nothing wrong with that option either, but you'd need to feel comfortable with that.

So to summarise, it all depends on what experience you want your users to have. In practice, most projects would probably make a compromise with something inbetween those two extremes, i.e. giving users some leeway to migrate their code but also eventually ensuring your codebase's technical debt is kept at a comfortable level.

[svanharmelen]

OK, I'll take it as is (with some minor style tweaks) 👍🏻