feat: Initial version of Save-AzDevOpsBuild #183
Conversation
|
Not sure if I need some other changes to incorporate this change. |
| exit 1 | ||
| } | ||
|
|
||
| exit 0 |
There was a problem hiding this comment.
I'm absolutely no Powershell expert, so if you have any improvements, recommendations, .... please let me know :)
fgheysels
changed the title
fgheysels
changed the title
|
I'm also open for suggestions for a better name for this function :) |
There was a problem hiding this comment.
Great addition! Thanks @fgheysels , great work. 🥇
I would ask you to add some unit tests:
- Stubbed response's status code to verify that the scripts fails
- Passing information results in an expected request
And an integration test, but we can take this up for you if you want:
- Setting the build flag on the current build,
- checking it's there in the assertion,
- and removing it back in the tear-down of the test.
|
|
||
| $requestBody = $retentionPayload | ConvertTo-Json -Depth 100 | ||
|
|
||
| $requestUri = "https://dev.azure.com/$OrganizationName/$ProjectId/_apis/build/builds/" + $BuildId + "?api-version=6.0" |
There was a problem hiding this comment.
It may be important to also make sure that legacy Azure DevOps systems can benefit from this (legacy = {organizationName}.visualstudio.com).
As I see it, you can benefit from the environment variable $env:SYSTEM_TEAMFOUNDATIONCOLLECTIONURI to retrieve the root URL where all Azure DevOps API calls can be made.
On the other hand, we may consider using a client package for this. But that may be taking things to far.
https://docs.microsoft.com/en-us/azure/devops/integrate/concepts/dotnet-client-libraries?view=azure-devops
There was a problem hiding this comment.
Is the API version also something we should parameterize? Default can be 6.0.
There was a problem hiding this comment.
I didn't want to parameterize the complete 'base' uri, as I thought it would make this more complex to pass this in as a parameter.
Unless you rely on this environment variable inside the script and do not pass it via a param.
There was a problem hiding this comment.
To be honest, I don't see a point right now in parametrizing the API version ? @mbraekman ?
There was a problem hiding this comment.
What we're typically doing, in case it's applicable, is set it as a parameter but default it to a specific version (as Stijn indicates). (for example in Enable-AzLogicApp)
The reason for this is that based on this approach the same script can easily be used to use the latest/another specific version of the API, without requiring any changes in the script.
However, while API versions change very often in the case of ARM templates, for instance, this doesn't seem to be the case for DevOps-related APIs. So, not quite sure if in this case, it would be very useful.
--> So not really required in this case.
Regarding the base-URI, I would rely on the built-in variables as much as possible, as this requires less input from the client using the script, increasing user-friendliness. But, even so, we should still offer users the possibility to provide their own custom values, just in case.
--> use built-in variable.
There was a problem hiding this comment.
But then I'd suggest that we do not pass in the organizationname parameter, but put the DevOps variable which contains the Uri inside the script ? This has the side effect that this script is only usable from within an Azure DevOps pipeline; that's why I opted for passing in the organisationName as a parameter, and not to rely on the System.CollectionUri variable which defines this base-url.
There was a problem hiding this comment.
Yes, exactly. So, we could opt for using the built-in base URL but that this is the default value for a custom provided URL?
There was a problem hiding this comment.
I don't think this would make it more useable. You'll have to pass in an url, consumer needs to know if it requires a trailing slash or not (ok, you can handle that in your script), but imho, it makes the whole thing a bit less user-friendly
There was a problem hiding this comment.
This one is a good example how we did it for another DevOps script:
https://github.com/arcus-azure/arcus.scripting/blob/master/src/Arcus.Scripting.DevOps/Scripts/Set-AzDevOpsArmOutputs.ps1
No, as I see it, all the necessary changes were made. Great job! 👍 |
Co-authored-by: stijnmoreels <[email protected]>
…els/arcus.scripting into frgh/feature/retain_build
There was a problem hiding this comment.
Great! Now we only need some tests. Could you provide some unit tests? We'll make sure that there's some integration test then if that's too much work for you.
A test I'm thinking on, is that the returned status code determines whether or not the script fails.
I've provided two tests in the Azure DevOps context. Don't know if that 's ok or if you want to have a different per script. I've tried |
fgheysels
changed the title
It would be nice if you could provide some integration tests indeed :) |
Ok, we'll look into that. |
Is that necessary to complete this PR ? I actually would like to use this asap :) |
There was a problem hiding this comment.
We'll make sure that there's some integration test then if that's too much work for you.
It would be nice if you could provide some integration tests indeed :)
Ok, we'll look into that.
Is that necessary to complete this PR ? I actually would like to use this asap :)
I guess not, we can make a preview package for you once this PR is completed. We can then take up the integration test ourselves. One remark I have is that it would be best to streamline this DevOps script with the other script we have:
https://github.com/arcus-azure/arcus.scripting/blob/master/src/Arcus.Scripting.DevOps/Scripts/Set-AzDevOpsArmOutputs.ps1
Take a look at how we did handled the subscription-specific information there. It can help resolve some discussions we had in this PR. Thanks again!
Oh, and I saw that while renaming the script, you didn't update the project file?
Once this is done, we can complete this, right @mbraekman ?
There was a problem hiding this comment.
- Streamline with
https://github.com/arcus-azure/arcus.scripting/blob/master/src/Arcus.Scripting.DevOps/Scripts/Set-AzDevOpsArmOutputs.ps1 - Update project file with renamed script
You mean making use of the DevOps variables like System.CollectionUri (https://docs.microsoft.com/en-us/azure/devops/pipelines/build/variables?view=azure-devops&tabs=yaml#system-variables-devops-services) I can do that, but imho it will make the script less usable (and maybe also harder to test ? ) Next to that, I can also rely on System.AccessToken and bake that into the script instead of having it to pass it in as a parameter, but also, this results in tight coupling the script with Azure Devops Pipelines. You'll not be able to use it in any other way. (Same goes for the Collectionuri variable. Don't you think the script will be to tightly coupled to Azure DevOps, as we're going to use DevOps specific variables inside the script ? |
Yeah, you definitely have a point. I think it's mainly for Azure DevOps intended, but let's ask @mbraekman bc I'm not sure 😅. I'll approve my part. |
|
Thanks again for your patience and time with this one! 🥇 |
As discussed, these scripts are meant to be used by/in Azure DevOps, so tightly coupling is not a problem, as long as we provide clear documentation on how/what it does. |
Added! |
A first version is created of a function that allows you to retain a DevOps build indefnitely.
This is based on the 'Builds' API of Azure DevOps, which allow you to update a build and set a 'keepForever' flag.
This results in a lease that is being created for the designated build. (I've also investigated the Leases API but then you have to specify an owner for that lease, whereas here you do not have to do that).
The Powershell function takes 2 arguments:
$(System.TeamProjectId)predefined variable$(Build.BuildId)variable.It relies on Azure DevOps variables to determine the URL of the project and to get an access-token that is necessary to do the API request
This relates to #182