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

Optional Download artifacts parameter #5158

Merged
merged 20 commits into from
Apr 8, 2020

Conversation

AlitzelMendez
Copy link
Member

Doing this change as a short term fix to unblock runtime to enable SDL runs.

@JohnTortugo
Copy link
Contributor

JohnTortugo commented Mar 30, 2020

NIT: Will anything work if the artifacts aren't downloaded? Can't they just disable the SDL validation?

@riarenas
Copy link
Member

This is #5149. Runtime repo wants to enable policheck. This is a tool that runs on source code and not artifacts, so it should be fine for that particular tool.

This looks OK to me in that you need to specifically opt out of downloading the artifacts, but it's a little scary in that it will break if they ever just want to add a tool that runs on the artifacts, as it's just a change in a variable group and might start breaking builds without any changes to their branch YAML, so it'll be really confusing.

@riarenas
Copy link
Member

I also don't know if we can look into the parameters that they provide to determine if they are running any artifacts based tools, and then warn about that so people are not surprised when it fails if they opted out of downloading assets?

@sunandabalu
Copy link
Member

NIT: Will anything work if the artifacts aren't downloaded? Can't they just disable the SDL validation?

Yes the artifact download is needed only for tools that run on artifacts, currently everyone is only running tools on source code. so having the ability to turn off artifact downloads def saves time. The extract step already has code in place to do nothing if the artifact folder is empty.

@sunandabalu
Copy link
Member

This looks OK to me in that you need to specifically opt out of downloading the artifacts, but it's a little scary in that it will break if they ever just want to add a tool that runs on the artifacts, as it's just a change in a variable group and might start breaking builds without any changes to their branch YAML, so it'll be really confusing.

Teams are setting the tools to run in the yaml directly, the only thing that is in the variable group is the user specific settings like area path, codebase admin coz every team has turned on the exact tools we asked them to :D So adding a artifacts based tool would require a PR to the yaml as of now but there is nothing stopping people to move this to the variable group as well.

@sunandabalu
Copy link
Member

I also don't know if we can look into the parameters that they provide to determine if they are running any artifacts based tools, and then warn about that so people are not surprised when it fails if they opted out of downloading assets?

To turn on tools running on artifacts is a separate parameter than the source-based one, so we can easily figure out if any artifact based tools have been added.

@sunandabalu
Copy link
Member

@AlitzelMendez Please run test build

  1. With the variable set to true in azure-pipeline.yml
  2. with the variable set to false in azure-pipeline.yml
  3. with nothing set for the variable.

@AlitzelMendez
Copy link
Member Author

Hi @sunandabalu

I changed a little bit the logic so always run unless the contrary be specified, sharing with you the test:

  1. With the variable set to true in azure-pipeline.yml
    https://dev.azure.com/dnceng/internal/_build/results?buildId=589299
  2. with the variable set to false in azure-pipeline.yml
    https://dev.azure.com/dnceng/internal/_build/results?buildId=588457
  3. with nothing set for the variable.
    https://dev.azure.com/dnceng/internal/_build/results?buildId=589787

in order to run this test I also included some changed in post-build.yml, as we don't want to change this in our build (pass that value) I am not sure if we need to include those changes.

@sunandabalu
Copy link
Member

sunandabalu commented Apr 6, 2020

in order to run this test I also included some changed in post-build.yml, as we don't want to change this in our build (pass that value) I am not sure if we need to include those changes.

@AlitzelMendez What did you have to change in post-build.yml?

@sunandabalu
Copy link
Member

I changed a little bit the logic so always run unless the contrary be specified, sharing with you the test:

  1. With the variable set to true in azure-pipeline.yml
    https://dev.azure.com/dnceng/internal/_build/results?buildId=589299
  2. with the variable set to false in azure-pipeline.yml
    https://dev.azure.com/dnceng/internal/_build/results?buildId=588457
  3. with nothing set for the variable.
    https://dev.azure.com/dnceng/internal/_build/results?buildId=589787

The tests looks like are doing the right thing, thanks for running them.

@AlitzelMendez AlitzelMendez self-assigned this Apr 7, 2020
@AlitzelMendez
Copy link
Member Author

@sunandabalu do you think that this change is ready to merge or needs more testing? or any change?

@sunandabalu
Copy link
Member

@sunandabalu do you think that this change is ready to merge or needs more testing? or any change?

you mentioned you changed something in post-build.yml to get the tests running, what was the change you had to make?

@AlitzelMendez
Copy link
Member Author

Hi @sunandabalu
I included the changes that we talked about

Copy link
Member

@sunandabalu sunandabalu left a comment

Choose a reason for hiding this comment

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

:shipit:

@AlitzelMendez AlitzelMendez merged commit b59ad40 into master Apr 8, 2020
@hoyosjs hoyosjs mentioned this pull request Apr 9, 2020
2 tasks
@riarenas riarenas deleted the almend/SLDOptionalDownloadArti branch June 15, 2020 15:19
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.

4 participants