gh-gei icon indicating copy to clipboard operation
gh-gei copied to clipboard

[ado2gh] RewirePipeline - Accept definitionId (int) for ado-pipeline parameter

Open tiago-soczek opened this issue 1 year ago • 4 comments

Description

Currently, we provide the name or path of the pipeline definition (ado-pipeline) to perform the rewire.

With this value, we look for the definitionId (int) - this is expensive as it is necessary to enumerate all Definitions as the API does not provide a method for this.

    // removed code for brevity
    public virtual async Task<int> GetPipelineId(string org, string teamProject, string pipeline)
    {
        // Check if we have the pipeline id cached - this will gone after process kill / execution
        if (_pipelineIds.TryGetValue((org.ToUpper(), teamProject.ToUpper(), pipelinePath.ToUpper()), out var result))
        {
            return result;
        }

        var url = $"{_adoBaseUrl}/{org.EscapeDataString()}/{teamProject.EscapeDataString()}/_apis/build/definitions?queryOrder=definitionNameAscending";
        
        // expensive call
        var response = await _client.GetWithPagingAsync(url);

To avoid this, the user could have the option to directly pass definitionId - this would avoid unnecessary calls and make the process faster.

The suggestion is to use the same parameter (ado-pipeline) and in the implementation, consider that it is the definitionId if it is a number, otherwise, follow the current process - convenient for those who do not have the definitionId at hand.

    // removed some code for brevity
    public virtual async Task<int> GetPipelineId(string org, string teamProject, string pipeline)
    {
        // Check if the pipeline is a number - the id itself
        if (int.ParseInt(pipeline, out var definitionId)) {
            return definitionId;
        }

        /// continue with the rest of the code

It makes sense? If so, I will provide a PR.

tiago-soczek avatar Feb 07 '24 19:02 tiago-soczek

@tiago-soczek I would suggest creating a new option --ado-pipeline-id instead seeing as we have a very specific use for --ado-pipeline to do the look up of the id.

brianaj avatar Feb 07 '24 20:02 brianaj

It also works, using two options it will be necessary to change the validation (--ado-pipeline OR --ado-pipeline-id is mandatory), but I think this is not a problem.

tiago-soczek avatar Feb 07 '24 20:02 tiago-soczek

Yeah that validation would live here.

brianaj avatar Feb 07 '24 20:02 brianaj