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

Throw error if the agent pool doesn't exist for Azure Pipelines scaler #2342

Closed
JorTurFer opened this issue Nov 25, 2021 Discussed in #2323 · 6 comments · Fixed by kedacore/keda-docs#612
Closed

Throw error if the agent pool doesn't exist for Azure Pipelines scaler #2342

JorTurFer opened this issue Nov 25, 2021 Discussed in #2323 · 6 comments · Fixed by kedacore/keda-docs#612
Assignees
Labels
Milestone

Comments

@JorTurFer
Copy link
Member

Discussed in #2323

Currently, the scaler doesn't throw any error if the poolId doesn't exist. Maybe we could validate if it exists IDK how to be more clear

@JorTurFer JorTurFer self-assigned this Nov 25, 2021
@tomkerkhove tomkerkhove changed the title Azure DevOps scaler: Throw an error if the pool doesn't exist Throw error if the agent pool doesn't exist for Azure Pipelines scaler Nov 25, 2021
@JorTurFer
Copy link
Member Author

I'm working on this task, and I'm not sure in the best way to proceed. I mean, validate if the pool exists or not requires an extra request to check it because the Azure DevOps API returns 200 despite not existing pool (crazy).

Maybe we could add support to specify directly the pool name instead of the pooID, if we are going to do an extra request to check if it exists, why don't get the ID from the name directly and use that to check if it exists?

We could also don't support it or maintain both ways but IMHO, using the pool name instead of the ID is more user-friendly

WDYT @kedacore/keda-contributors ?

@JorTurFer
Copy link
Member Author

JorTurFer commented Nov 30, 2021

These endpoints return the same json:

So maybe maintain the old method and add the new one it's not a drama (but again, honestly as a user I prefer to use pool name instead of a weird ID also despite the breaking change)

@JorTurFer
Copy link
Member Author

I have to add tests and update e2e test also but my idea is something like this: #2370

@tomkerkhove
Copy link
Member

These endpoints return the same json:

Can you share a payload?

So maybe maintain the old method and add the new one it's not a drama (but again, honestly as a user I prefer to use pool name instead of a weird ID also despite the breaking change)

Let's support both, end-users can then choose and most likely they will all use the name but let's give people options.

@JorTurFer
Copy link
Member Author

Okey, I will take a look maintianing both :)
Related with the payload, I will add an example in half an hour (now I'm AFK)

@JorTurFer
Copy link
Member Author

JorTurFer commented Dec 1, 2021

{
   "count":1,
   "value":[
      {
         "createdOn":"2021-03-03T10:30:47.017Z",
         "autoProvision":false,
         "autoUpdate":true,
         "autoSize":null,
         "targetSize":null,
         "agentCloudId":null,
         "createdBy":{
            "displayName":"xxxx",
            "url":"https://spsprodeus27.vssps.visualstudio.com/Axxxxxa4d-47xx-458b-be3c-xxxxxxxxxxxxxx/_apis/Identities/04b3xxxx-dcf1-68a5-xxxx-3be81f93f076",
            "_links":{
               "avatar":{
                  "href":"https://dev.azure.com/YYYYYY/_apis/GraphProfile/MemberAvatars/aad.MDRiM2IwOTItxxxxMS03OGE1LWEwZGEtM2JlODFmOTNmMDc2"
               }
            },
            "id":"04b3b092-dcf1-68a5-a0da-3be81f93f076",
            "uniqueName":"xxxxxxxxx@xxxxxx.com",
            "imageUrl":"https://dev.azure.com/yyyyyyy/_apis/GraphProfile/MemberAvatars/aad.MDRiM2IwOTItZGNmMS03OGE1LWEwZGEtM2JlODFmOTNmMDc2",
            "descriptor":"aad.MDRiM2IwOTItZGNmMS03OGE1LWEwZGEtM2JlODFmOTNmMDc2"
         },
         "owner":{
            "displayName":"xxxxx",
            "url":"https://spsprodeus27.vssps.visualstudio.com/Ac66d5a4d-4775-458b-be3c-xxxxxxxxxxxxxx/_apis/Identities/04xxxx92-dcf1-xxxx-a0da-3xxxxxxf076",
            "_links":{
               "avatar":{
                  "href":"https://dev.azure.com/yyyyyyyy/_apis/GraphProfile/MemberAvatars/aad.MDRiM2IwOTItZGNmMS03OGE1LWEwZGEtM2JlODFmOTNmMDc2"
               }
            },
            "id":"04b3b092-dcf1-68a5-xxxx-xxxxxxf076",
            "uniqueName":"xxxxx@xxxxx.com",
            "imageUrl":"https://dev.azure.com/yyyyyyy/_apis/GraphProfile/MemberAvatars/aad.MDRiM2IwOTItZGNmMS03OGE1LWEwZGEtM2JlODFmOTNmMDc2",
            "descriptor":"aad.MDRiM2IwOTItZGNmMS03OGE1LWEwZGEtM2JlODFmOTNmMDc2"
         },
         "id":19,
         "scope":"48953491-beb9-42fe-99af-xxxxxxxx",
         "name":"k8s-deploy",
         "isHosted":false,
         "poolType":"automation",
         "size":2,
         "isLegacy":false,
         "options":"none"
      }
   ]
}

But we only need this:

{
   "count":1,
   "value":[
      {
         "id":19,
      }
   ]
}

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

Successfully merging a pull request may close this issue.

2 participants