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

Remove copy instructions in PushToAzureDevOpsArtifacts #4565

Merged
merged 6 commits into from
Jan 10, 2020
Merged

Remove copy instructions in PushToAzureDevOpsArtifacts #4565

merged 6 commits into from
Jan 10, 2020

Conversation

JohnTortugo
Copy link
Contributor

@JohnTortugo JohnTortugo commented Dec 24, 2019

Closes: #4564

This is the first of two PRs. Second one will be to remove the copy instructions in the SDK Publish.proj.

This change and also the one in the SDK have been tested here: https://dnceng.visualstudio.com/internal/_build/results?buildId=465204&view=results https://dnceng.visualstudio.com/internal/_build/results?buildId=465001&view=results

@JohnTortugo JohnTortugo self-assigned this Dec 24, 2019
Copy link
Member

@riarenas riarenas left a comment

Choose a reason for hiding this comment

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

Happy that we don't need this anymore.

Copy link
Member

@riarenas riarenas left a comment

Choose a reason for hiding this comment

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

Hmm, on second thought I wonder if we should wait a bit before pushing this to repos, We made sure that Arcade doesn't move the artifacts around anymore, but if someone else does it as well, this would end up breaking official builds without any warning in the PR.

@mmitche
Copy link
Member

mmitche commented Jan 6, 2020

Hmm, on second thought I wonder if we should wait a bit before pushing this to repos, We made sure that Arcade doesn't move the artifacts around anymore, but if someone else does it as well, this would end up breaking official builds without any warning in the PR.

@JohnTortugo Is there any way to roll out an update that would flag this initially if repos are doing something bad?

@JohnTortugo
Copy link
Contributor Author

Hmm, on second thought I wonder if we should wait a bit before pushing this to repos, We made sure that Arcade doesn't move the artifacts around anymore, but if someone else does it as well, this would end up breaking official builds without any warning in the PR.

@JohnTortugo Is there any way to roll out an update that would flag this initially if repos are doing something bad?

I think unfortunately there isn't much we can do. The potential issue here is for files be moved/deleted after we emit the ##vso command but before AzDO actually execute the upload. So, I think a better error message in the ##vso implementation would be the right thing.

That being said, I added some File.Exists conditions in the task. That will at least tell us that the file existed at the right place at some point in time (or not).

What do you think?

@riarenas
Copy link
Member

riarenas commented Jan 7, 2020

@JohnTortugo Is there any way to roll out an update that would flag this initially if repos are doing something bad?

Maybe we can make this in two stages:

  1. Log something if we see that assetsTemporaryDirectory is being passed to the task. Look for those logs in repos that have a publishing.props file somewhere (Which I think would be the only place to actually customize this behavior)
  2. Second change that actually removes the property from the task after letting people know.

Or maybe we can simply remove the usage of the property from Arcade and keep it in the task?

@JohnTortugo
Copy link
Contributor Author

JohnTortugo commented Jan 7, 2020

Talked offline. I'll remove the usage of the property in the task & SDK and change the task to print a message saying that the property is deprecated if it's used.

…ifacts.cs

Co-Authored-By: Ricardo Arenas <riarenas@microsoft.com>
@JohnTortugo JohnTortugo merged commit 0f082e6 into dotnet:master Jan 10, 2020
@JohnTortugo JohnTortugo deleted the DontCopyFiles branch January 10, 2020 18:27
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.

Remove copy instructions in PushToAzureDevOpsArtifacts
3 participants