Hi @tigerwnz I created this issue to remove support for ID in the "GET agent" REST API as you suggested (!133681 (comment 1601344022)), as I didn't want to go off topic on the MR.
My concern about removing support for ID is that we will also have to change the frontend. Also, would this be consistent with other APIs? I am only familiar with the Project and Group APIs and they support both ID and fully-qualified path, although that being said, they do not have the same naming issue as the Cluster Agent.
@partiaga I'm undecided about this one. Changing the frontend should be fine, but several other objects with names (eg. milestones, labels) are found by ID, so we're actually adding inconsistency by using name (though this is not to take away from the value of searching by name for agents, as this is a far better experience for Terraform).
Since we have to wait for a major release anyway, perhaps instead of deciding right away we could wait and see if we get any complaints from users about the ID/name collision? The edge case is certainly technical debt, but we may find it doesn't actually cause any real problems. WDYT?
Also cc @nagyv-gitlab if you have any preference from a product point of view.
@partiaga@tigerwnz I think ID was added to the UI specifically to help with troubleshooting to identify an agent registration instance from the logs. I think @ash2k can validate this.
Getting agent by it's ID can be also very useful. Sometimes you don't have the name, you only have an ID (like in logs). What are we going to do if we need to get agent by ID if we remove this functionality?
I don't agree with the approach in #427522. I think introducing type confusion is not the best approach. (In general I think query parameters is what should be used instead of embedding things into URL path because... they have been invented for this exact purpose.) But anyway, why don't we introduce a query parameter to get agent by name and leave the fetch-by-ID code alone?
I think introducing type confusion is not the best approach.
Yeah, it's not ideal, but I think the suggestion in #427522 is just basing on the other APIs, e.g.: the "GET project" API accepting both ID and the project's fully-qualified name (with the namespace).
prioritize the ID over name when querying, and to document that behavior.
This is just a surprise for later for future users Someone might run into this in a bad way sooner or later. E.g. find the agent and delete it - can delete a wrong agent because of the id vs name confusion. I really don't understand what's wrong with just using a separate way to specify the name vs id? I think safe API is more important than consistent API.
Also, suppose the user saw the warning in the doc about the unfortunate API behavior. What is the user supposed to do in their code that calls this API? Check if the name is all digits and then what? There is no fallback. So e.g. Terraform provider would... error out on digit-only names? That way they effectively become unsupported. But they are allowed, why are they not supported?
Note that we cannot change that even if nobody uses digit-only names on GitLab.com because of self-managed customers (we don't know what they are doing). Doing this as a breaking change and forcing everyone to rename (names are immutable, cannot rename) recreate all their incompatible agents is... not worth it?
Please don't treat me as the DRI on this decision, I'm not. I suggest to check with someone who is familiar with how all GitLab REST API works and figuring out what is the best way forward here. What I've said above is just my personal opinion
I really don't understand what's wrong with just using a separate way to specify the name vs id?
There is nothing wrong with it. My suggestion was to do it as the next iteration
I suggest to check with someone who is familiar with how all GitLab REST API works and figuring out what is the best way forward here.
Yes, I agree, that's why I think this isn't something we can do right away. What I mean is that, we can't make the contributor writing the MR (!133681 (closed)) pivot right away because we need to think about it.
What is the user supposed to do in their code that calls this API? Check if the name is all digits and then what? There is no fallback. So e.g. Terraform provider would... error out on digit-only names?
I'm not sure I understand what you mean by this?
In the proposed change and the MR, the priority is to query by ID but for example if the user sends a numeric parameter and the ID does not exist, it then queries by name.
I do understand your other concerns about safety. But given that this endpoint also filters out by project (ie it's never a global query of all agents), I thought there wouldn't be much chance of a collision. That's why I thought it would be acceptable when I reviewed the contributor's MR.
There is nothing wrong with it. My suggestion was to do it as the next iteration
But then we'll have two ways to do this? Why do we need both? I see downsides but don't see a single upside to keeping the approach suggested in that MR if we can have a safe API for this.
Yes, I agree, that's why I think this isn't something we can do right away.
Is there urgency to merge this MR? We'll be stuck with this API for a long time (at least until %17.0 if we deprecate it right after adding), I reckon it's worth getting it right from get go.
What I mean is that, we can't make the contributor writing the MR (!133681 (closed)) pivot right away because we need to think about it.
Why not delay merging if we need to think about it? Why merge if we are not sure? Changing the behavior later is not free, it's a lot of work that one of us will have to do. And anyone who might have started using the new API will also have to change their code.
Just to double check, @nagyv-gitlab you made the comment here to support querying by agent name from the CLI (which uses the API), does this have any urgency?
We are not allowed to make any breaking changes in the REST API (at least during v4) - see here.
Therefore, I suggest to close this issue and somehow still try to support accessing by ID as a first priority and (with limitations) support accessing by name.