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

Normalize Azure devops URIs during asset manifest creation #7493

Closed
1 of 2 tasks
riarenas opened this issue Jun 9, 2021 · 5 comments · Fixed by #7521
Closed
1 of 2 tasks

Normalize Azure devops URIs during asset manifest creation #7493

riarenas opened this issue Jun 9, 2021 · 5 comments · Fixed by #7521
Assignees
Labels
Detected By - Ad-Hoc Testing Issue was detected by ad-hoc testing preformed by someone on the engineering team Ops - First Responder

Comments

@riarenas
Copy link
Member

riarenas commented Jun 9, 2021

At some point it looks like Azure devops decided to change the format of their URIs, as an example, the format of the repository URI build variables changed from: https://dnceng.visualstudio.com/internal/_git/repo-name to https://dnceng@dev.azure.com/dnceng/internal/_git/repo-name

When a build runs some of its legs in machines that have an old enough agent version to use the old URI format, we are hitting errors when publishing to the BAR, because we need all the manifests produced by the legs to have this information in sync.

We should add some normalization logic to the build manifest creation to guard ourselves against these kinds of changes.

@ghost ghost added the Needs Triage A new issue that needs to be associated with an epic. label Jun 9, 2021
@riarenas
Copy link
Member Author

riarenas commented Jun 9, 2021

We've only seen this once ever, but it seems like the kind of thing that can bite us unexpectedly if the Azure devops folks decide they want a new format for the URIs again, so I think this should go into FR.

@ilyas1974 ilyas1974 added Detected By - Ad-Hoc Testing Issue was detected by ad-hoc testing preformed by someone on the engineering team and removed I Think This Is Triaged Needs Triage A new issue that needs to be associated with an epic. labels Jun 10, 2021
@michellemcdaniel
Copy link
Contributor

Looks like we could just take the NormalizeUrl function from the AzureDevOpsClient in DarcLib and port it to arcade. I assume we don't want any dependencies in arcade on darclib, correct?

@michellemcdaniel michellemcdaniel self-assigned this Jun 14, 2021
@riarenas
Copy link
Member Author

I'm not sure any changes would need to be made in Arcade. The failing task lives in Arcade-services: https://github.com/dotnet/arcade-services/blob/main/src/Maestro/Microsoft.DotNet.Maestro.Tasks/src/PushMetadataToBuildAssetRegistry.cs

@michellemcdaniel
Copy link
Contributor

Oh, I thought the issue was with the actual creation of the individual manifests? Do we not care if they're normalized there?

@riarenas
Copy link
Member Author

I personally don't see a problem with the individual manifests using different versions of the URI as long as the task understands that they are the same. Nothing should be processing them individually anyways. Not sure if there's any pushback in that regard.

If we want to normalize ASAP (when the Build object gets created) then yeah, the code should go into arcade, and it should probably not introduce a dependency on darclib.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Detected By - Ad-Hoc Testing Issue was detected by ad-hoc testing preformed by someone on the engineering team Ops - First Responder
Projects
None yet
3 participants