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

Enable Policheck #33841

Closed
wants to merge 2 commits into from
Closed

Enable Policheck #33841

wants to merge 2 commits into from

Conversation

Anipik
Copy link
Contributor

@Anipik Anipik commented Mar 20, 2020

I have scheduled an official build to test the change as well

Copy link
Member

@safern safern left a comment

Choose a reason for hiding this comment

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

LGTM as long as the official build is green.

@dagood
Copy link
Member

dagood commented Mar 20, 2020

I have scheduled an official build to test the change as well

I highly recommend linking to it from this PR to make it easy to look at the results.

@stephentoub
Copy link
Member

@Anipik, should this be merged?

@safern
Copy link
Member

safern commented Mar 31, 2020

I think this should be addressed first: #33841 (comment)

It is really helpful for the comment to link to the variable group.

@Anipik
Copy link
Contributor Author

Anipik commented Mar 31, 2020

It is really helpful for the comment to link to the variable group.

i moved it to here. https://github.com/dotnet/runtime/pull/33841/files#diff-2601d29fabf2d8fa57d58b3eb4ff8d17R28 The variable group is still not recognised , can you suggest the appropriate place ?

@Anipik, should this be merged?

This post build stage is still not getting passed on the actual build https://dev.azure.com/dnceng/internal/_build/results?buildId=579800

i am working with the arcade folks to get it fixed.

@safern
Copy link
Member

safern commented Mar 31, 2020

i moved it to here. https://github.com/dotnet/runtime/pull/33841/files#diff-2601d29fabf2d8fa57d58b3eb4ff8d17R28

Right, but this won't work because the post-build.yml template doesn't take any variables parameters, and it doesn't flow it to the underlying templates, so it is just getting lost when expanding that template.

The variable group is still not recognised , can you suggest the appropriate place ?

You need to add it to this section which is where we declare the yml variables:
https://github.com/dotnet/runtime/blob/master/eng/pipelines/runtime-official.yml#L23

Once you put it there it should work. Since it will be in a different template, add the comment back referencing the variable group. Also, since runtime-official.yml is only used in official builds and not used in PRs or dnceng/public you shouldn't need to add the condition to check that we're in internal.

@ViktorHofer
Copy link
Member

@Anipik we want to port this into preview3 as just discussed in Tactics.

@Anipik
Copy link
Contributor Author

Anipik commented Mar 31, 2020

linking the issues
dotnet/arcade#5149
dotnet/arcade#5168

Copy link
Member

@jaredpar jaredpar left a comment

Choose a reason for hiding this comment

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

Doing this in official builds seems like the wrong approach. We should be catching this at PR time.

eng/pipelines/runtime-official.yml Show resolved Hide resolved
@@ -29,6 +29,12 @@ jobs:
pool:
name: Hosted VS2017
steps:
- task: UseDotNet@2
Copy link
Member

Choose a reason for hiding this comment

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

This should be done in arcade instead, this is part of eng/common/*.

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 they will be taking in the change as well, if the solution solves the problems

Copy link
Member

@jaredpar jaredpar left a comment

Choose a reason for hiding this comment

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

Removing the reject for now as this is apparently the intended path forward. Going to be following up separately on removing this from official builds as this will seemingly reduce official build reliability. Want us to persue other alternatives here, rolling builds, separate pipelines in the internal project or ideally PR validation.

@ViktorHofer
Copy link
Member

@Anipik what's the status on this?

@safern
Copy link
Member

safern commented Apr 23, 2020

@Anipik it seems like we no longer need this given @jcagme's email from yesterday? Instead we need to add the sdl config file on .eng?

@Anipik Anipik closed this Apr 28, 2020
@Anipik Anipik deleted the policheck branch May 8, 2020 19:49
@ghost ghost locked as resolved and limited conversation to collaborators Dec 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants