Skip to content

Remove agent.os demands on windows ci #621

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

Merged
merged 3 commits into from
Aug 2, 2018
Merged

Conversation

safern
Copy link
Member

@safern safern commented Jul 31, 2018

All the machines in this pool are guaranteed to be Windows_NT and now that the pool was made big enough not all of them define an agent.os property, so if they don't have one it will only use the machines that define that only. So in order to have more machines available and make CI faster, let's remove that statement and bump parallel to 4.

This was causing Windows builds to be slow and not ran in parallel.

cc: @chcosta @eerhardt

@chcosta
Copy link
Member

chcosta commented Jul 31, 2018

VSTS intends to fix the parallel property to allow something like a "*". in the mean time, they suggest just setting it to something like '99' so you don't have to twiddle with it any time you make a change to the matrix.

@Ivanidzo4ka
Copy link
Contributor

I know it has nothing to do with your PR, but maybe you familiar with issue, or know where to report it.
#528
if you click on View Details for merge commit - VSTS: public-CI in progress, and if you click on details - all 4 runs are complete. Do you know why? And where to report this behavior?

@safern
Copy link
Member Author

safern commented Aug 1, 2018

What I've been seen is that when you merge before it reports the status or if you push another commit it just unlinks that badge from the build. But @chcosta should know more.

Copy link
Member

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

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

Lgtm

@shauheen
Copy link
Contributor

shauheen commented Aug 1, 2018

I know this is build related and relatively minor change, but I think we should file and associate an issue with it nonetheless. @eerhardt what do you think?

@eerhardt
Copy link
Member

eerhardt commented Aug 1, 2018

On the .NET Core team, we typically don't require issues to be opened when making immediate, small infrastructure changes. As long as the PR has a good description of the problem being addressed, we just create PRs with a good description.

However, we can manage the machinelearning repo however we want.

My personal opinion here is that I wouldn't require an issue. It wouldn't have any more information/benefit than what is already in the PR.

@Ivanidzo4ka
Copy link
Contributor

It was in done state internally for over an hour, and then I lost my patience and just merge changes. And this is I think 2nd time I see this behaviour, but previously I just push another commit to PR.


In reply to: 409410507 [](ancestors = 409410507)

@safern
Copy link
Member Author

safern commented Aug 1, 2018

It was in done state internally for over an hour, and then I lost my patience and just merge changes.

I think that good be more of a bug in VSTS or Github in the way they plugin or update the state of the badges. @chcosta have you heard about a known issue causing this? If not could you help filing the issue accordingly to the VSTS team?

@safern
Copy link
Member Author

safern commented Aug 1, 2018

My personal opinion here is that I wouldn't require an issue. It wouldn't have any more information/benefit than what is already in the PR.

I agree with this, I think in such a small change not impacting the product itself, it would take me more time structuring the issue accordingly and opening it, than what it actually took me to fix it and open the PR. However if the repo owners which are you guys decide that want to have issue per PR, I do respect that and will do so for the next time I find an issue like this one.

@chcosta
Copy link
Member

chcosta commented Aug 1, 2018

This is a known issue which I've discussed with VSTS. They're tracking an issue to fix it, but I didn't have anything on our side tracking the status (my bad). I've filed https://dotnet.visualstudio.com/7ea9116e-9fac-403d-b258-b31fcf1bb293/_workitems/edit/58, and I'll follow up with VSTS to get a better understanding of where the work is at.

@eerhardt
Copy link
Member

eerhardt commented Aug 1, 2018

@safern - can you also update the official build definition with this as well?

- agent.os -equals Windows_NT

@safern
Copy link
Member Author

safern commented Aug 2, 2018

@safern - can you also update the official build definition with this as well?

After speaking with the engineering team this is not possible as we have to stick with the current queue in order to be able to sign.

@eerhardt who else could review and sign off in this PR?

@eerhardt eerhardt requested a review from Ivanidzo4ka August 2, 2018 19:27
Copy link
Contributor

@Ivanidzo4ka Ivanidzo4ka left a comment

Choose a reason for hiding this comment

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

:shipit:

@safern safern merged commit b21094d into master Aug 2, 2018
@safern safern deleted the RemoveDemandsWindowsCi branch August 2, 2018 20:01
@ghost ghost locked as resolved and limited conversation to collaborators Mar 29, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants