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

DownloadBuildArtifacts - Use latest build filtered by branch #6356

Merged
merged 11 commits into from
Mar 22, 2018

Conversation

keesschollaart81
Copy link
Contributor

Implements #6076

2018-02-02 15_56_38-msi - visual studio team services

@GitHubSriramB
Copy link
Contributor

@keesschollaart81

  1. It would be better to let user select "Latest" as value for Build version & "Any" or "*" for the branch rather than letting them be empty.
  2. We should have Branch input appear before Build so that Builds are listed from the selected branch.

@keesschollaart81
Copy link
Contributor Author

Thanks, great ideas! I'll try to implement them next week and update the PR!

@keesschollaart81
Copy link
Contributor Author

Just updated the PR with an updated UI. Regarding 2), I chose a slightly different approach by using a combo to only show one of the two inputs (branche-name or build-id) because you would never use them in conjunction.

vsts-download-build-artifacts

Let me know if you find this better or if I can still improve the UI experience / labels...

@GitHubSriramB
Copy link
Contributor

GitHubSriramB commented Feb 20, 2018

@keesschollaart81 -

  • Rename "Latest" to "Latest build from specific branch"?
  • Rename "Specific buildnumber" to "Specific build"?

@@ -126,12 +128,28 @@ async function main(): Promise<void> {
// Triggering build info not found, or requested, default to specified build info
projectId = tl.getInput("project", true);
definitionId = definitionIdSpecified;
buildId = parseInt(tl.getInput("buildId", true));
buildId = parseInt(tl.getInput("buildId", false));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should make sure buildId is specified when the option chosen is specific build.

}
}

// verify that buildId belongs to the definition selected
if (definitionId) {
if (!buildId){
// get latest successful build filtered by branch
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should do this only when "latest build" option is chosen

@keesschollaart81
Copy link
Contributor Author

Agree! Done!

@leidegre
Copy link
Contributor

leidegre commented Mar 8, 2018

Any idea when this will be available for everyone generally? Killer feature for my use case right now and something I've been missing from VSTS.

@ashokirla
Copy link

@keesschollaart81
This is a great feature and we love it. Just few recommendations:
Instead of using radio buttons for “Download artifacts produced by” , I would suggest we have similar experience we have in our artifacts linking. See
artifacts

Instead of using “Download artifacts produced by” parameter with radio buttons, we can have “Artifact version” as the parameter name with a drop down. And the dropdown can have below entries.

  1. Latest
  2. Latest from specific branch
  3. Specific version.
    Rest of the experience stays the same.
    This way the experience will be consistent across. And going forward if we chose to add more options, this design will scale.

Discussed the same with @GitHubSriramB and this will be the final set of changes :).

@keesschollaart81
Copy link
Contributor Author

Great! Like it, done!

@GitHubSriramB
Copy link
Contributor

GitHubSriramB commented Mar 14, 2018

@keesschollaart81 - one last comment reg. the name for the name for the version input. Instead of naming it "Artifact version", can you name it as "Build version to download"? Since there are artifacts within a Build & the version we are providing as input is for the Build that is applicable for all the artifacts, wanted to avoid naming it as "Artifact version" & causing any confusion.

already discussed this suggestion with @ashokirla and he is fine with this too.

console.log(tl.loc("LatestBuildFound", buildsForThisDefinition[0].id));
buildId = buildsForThisDefinition[0].id
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we reuse the build we got in case version is latest or latestFromBranch instead of calling getBuild again in line 156?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, nice catch! Implemented & tested!

@keesschollaart81
Copy link
Contributor Author

Again... 👍

@@ -68,9 +68,9 @@
"helpMarkDown": "If checked, this build task will try to download artifacts from the triggering build. If there is no triggering build from the specified definition, it will download artifacts from the build specified in the options below."
},
{
"name": "artifactVersion",
"name": "buildVersionToDownload",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

folks only see the label. Is there a reason to rename the name? It will cause the tasks to get redownloaded since I believe the folder is by name + guid.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does this mean to yaml and all our docs and templates? Is this rename really worth it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a naming change of a field that is added as part of this feature (initially implemented by me, named 'artifactVersion'). On top of that, this is only a renaming of this parameter, not the renaming of the whole step. I dont think this is breaking in any way. With this, I dont see how this affects the yaml/docs... Or am I missing something here?

console.log(tl.loc("LatestBuildFound", build.id));
buildId = build.id
}
if (!build){
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

leave blank line after previous if block

@drewkg
Copy link

drewkg commented Mar 20, 2018

With Sprint 131 now being rolled out, the addition of this capability as a build task would significantly reduce the number of custom scripts that are currently in place to perform the same task.

@omeshp omeshp merged commit ca7ab3d into microsoft:master Mar 22, 2018
@omeshp
Copy link
Contributor

omeshp commented Mar 22, 2018

@drewkg This feature will be deployed with M133 changes.

@drewkg
Copy link

drewkg commented Mar 22, 2018

Thanks for the timeline.

@leidegre
Copy link
Contributor

@omeshp what does M133 mean? Where can I find a reference to that, or an actual time frame?

@madhurig
Copy link
Contributor

@leidegre: VSTS deploys fixes and features generally on a 3 week cadence/sprints. Currently deployed version in production is M131 (milestone 131): https://docs.microsoft.com/en-us/vsts/release-notes/2018/mar-05-vsts.

M133 will start deploying after April 15th. We do a staged rollout - generally within 2 weeks all the VSTS accounts will have the updates.

Thanks,
Madhuri

@steve-hawkins
Copy link

The version 0.131.1 of Download Build Artifacts shows the Build picklist as version numbers in the Release Definition UI and currently does not appear to allow variables to be used as the 'buildId' input:-

image

specify a variable with target version number:-
download_version=1.0.14.1

image

==============================================================================
Task         : Download Build Artifacts
Description  : Download Build Artifacts
Version      : 0.131.1
Author       : Microsoft Corporation
Help         : 
==============================================================================
##[error]Build with id NaN not found for build definition id [REDACTED]

can you confirm if the 'buildVersionToDownload' input in this version of Download Build Artifacts will accept a version number variable?

@doormalena
Copy link

doormalena commented Mar 27, 2018

Is it possible somehow to obtain the value of the "build.sourceLabel" artifact (preferably available as a variable) together with the new option to always use the latest build version available?

Background: we have a CI build publishing an artifact. Using another build definition we collect this artifact (using this build task) and generate a product package (MSI etc.). We need the version number of the CI build output to be set as the version number of this build definition (and the MSI). I was thinking of using ##vso[build.updatebuildnumber] for this accompanied with the value of "build.sourceLabel".

@gertvdh
Copy link
Contributor

gertvdh commented Apr 24, 2018

This got installed on our account this week, this increases our productivity, thanks for the effort!

@thecontrarycat
Copy link

Is there any way to update this task for OnPrem TFS 2018? I really need this feature to fix our releases...

@romil07
Copy link
Contributor

romil07 commented Jul 2, 2018

@thecontrarycat
You can have this update but following scenario won't work for OnPrem TFS 2018:

The checkbox "When appropriate, download artifacts from triggering build" option won't work for you. This was released after the TFS 2018 was shipped and hence, it's available only for the online users as of now or the TFS 2018 update 2 users.

If you are okay with this, let me know. I will give you the steps to update the task.

@t3mi
Copy link

t3mi commented Aug 22, 2018

@keesschollaart81
Any chance you can add BuildResult.PartiallySucceeded as an additional option for the filtering when you searching for builds (Line 143)? Or at least please change the inputs/docs that you are searching for the latest succeeded build.

@keesschollaart81
Copy link
Contributor Author

I don't know, what do @GitHubSriramB or @ashokirla think? I wont be able to implement this in the next 6 weeks, at least. Why don't you pop a PR yourself? ;-)

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

Successfully merging this pull request may close these issues.