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

Clean version files and update Maestro task version #1950

Merged
merged 1 commit into from
Feb 1, 2019
Merged

Clean version files and update Maestro task version #1950

merged 1 commit into from
Feb 1, 2019

Conversation

jcagme
Copy link
Contributor

@jcagme jcagme commented Feb 1, 2019

Fixes #1949

@@ -10,10 +10,6 @@
<Uri>https://github.com/dotnet/arcade</Uri>
<Sha>1e859f1c17fffbe9c4fb6bbfc0fc71cd0c56563b</Sha>
</Dependency>
<Dependency Name="Microsoft.DotNet.Maestro.Tasks" Version="1.0.0-beta.19060.8">
Copy link
Member

Choose a reason for hiding this comment

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

Poking my nose in... shouldn't this be taking auto-updates from arcade-services, if this library is built there now?

Is the hard-coded version in Versions.props really hard-coded and manually maintained, or am I misunderstanding?

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, when the auto update is done we touch eng/Versions.props, eng/Version.Details.xml and global.json. The version of Microsoft.DotNet.Maestro.Tasks is now defined in src/Microsoft.DotNet.Arcade.Sdk/tools/SdkTasks/Versions.props which is not meant to be auto updated. So there are really two options:

  1. Make src/Microsoft.DotNet.Arcade.Sdk/tools/SdkTasks/Versions.props import eng/Version.props and if MicrosoftDotNetMaestroTasksVersion is defined there, use it. This approach will need that all repos add an entry in Version.Details.xml and Versions.props for Microsoft.DotNet.Maestro.Tasks
  2. Update the hard coded version manually after there is a change

Given the churn of this library I'll go with 2 for now and it this changes we could look into 1

Copy link
Member

Choose a reason for hiding this comment

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

We intentionally don't want the sdk tasks projects importing things from elsewhere, particularly from the repo. All of the recent refactoring was to get away from repo weirdness affecting something that's so autonomous. Darc should be taught to update tools/SdkTasks/Versions.props

Copy link
Member

Choose a reason for hiding this comment

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

Oh! I missed that tools/SdkTasks/Versions.props is included as-is in the Arcade SDK. Yeah, that makes things more complicated.

I'd add (3): generate this file during the Arcade build based on the current version in eng/Version.props. This makes the dependency auto-upgrade, but still be static per Arcade SDK build so repos can't introduce mismatch problems by doing unexpected upgrades.

Using (2) for now does seem pragmatic, and I'm fine with that.

Copy link
Member

Choose a reason for hiding this comment

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

Missed @chcosta's comment when posting mine, I agree adding that kind of dependency expression also sounds fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, #1956

@jcagme jcagme merged commit ab01877 into dotnet:master Feb 1, 2019
@jcagme jcagme deleted the updateMaestroTask branch February 1, 2019 23:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants