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

Set up CI with Azure Pipelines yml #40

Merged
merged 28 commits into from
Nov 2, 2018
Merged

Conversation

azure-pipelines[bot]
Copy link

No description provided.

@gusty gusty changed the title [WIP] Set up CI with Azure Pipelines yml Set up CI with Azure Pipelines yml Nov 2, 2018
@bartelink bartelink force-pushed the master branch 3 times, most recently from 53ddf1c to fa78fca Compare November 2, 2018 10:29
@@ -22,8 +22,8 @@
<Target Name="Test">
Copy link
Collaborator

@enricosada enricosada Nov 2, 2018

Choose a reason for hiding this comment

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

some random notes:

  • rename the target build as Pack, because inside there you do dotnet pack
  • DefaultTargets="Test;Build seems strange, these two target are not dependent on each other, so do not trust the order. so in the build script, run the dotnet msbuild /t:Build and a dotnet msbuild /t:Test separately
  • refactoring the name property $(Name) ihmo is annoying, make things harder to read. i understand copy past, but really every repo is different anyway

Copy link
Collaborator

Choose a reason for hiding this comment

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

  • agree with renaming to Pack, but was assume callers were going to do dotnet build etc. One thing this does not cover is if a repo has stuff to Test, Build and Pack - I was using this Build doing the pack, and msbuild doing Test+Build hack as a way to make the yaml copy-pasteable/diffable.
  • was not 'trusting' the order, but per msbuild it does define the order, so a dotnet msbuild tuns tests first this way, which is what you want
  • I'm kinda fond of the $(Name) (it was not my invention), but agree it should go

I won't fix these in this PR; feel free to if you happen to have time and inclination (I'll do this to CallPolly and Jet.JsonNet.Converters as a batch)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Making things explicit help, more so with things not so commonly used (less magic).

  • dotnet msbuild build.proj ppl need to know about DefaultTargets.
  • dotnet msbuild /t:Pack build.proj is explicit.

And the yaml can just dotnet msbuild /t:Pack build.proj, or dotnet pack build.proj
and after that, dotnet msbuild /t:Test
and adding a <Target Build just to dotnet build is small too

Ihmo The yaml file is not a part who should be copy pasted as is, it's a part of this repo, and can and will be different on other repos (everyone is a bit differnt anyway). we can share and minimize diff, without creating hidden conventions.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree with all your points; thanks for laying them out so clearly. I'll apply that philosophy in each of the repos at a later point ;)

@bartelink bartelink merged commit 523c96e into master Nov 2, 2018
@bartelink bartelink deleted the gus-azure-pipelines-yml branch November 2, 2018 12:41
@bartelink bartelink mentioned this pull request Nov 2, 2018
bartelink pushed a commit that referenced this pull request Nov 4, 2018
* Set up CI with Azure Pipelines yml
* Add short term skipping of tests
* Workaround xunit xplat issues
* Fix path case for Linux
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