Move telemetry source configuration to pipeline variables - #698
Conversation
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
Updated after getting more context on the goal here. My original comment suggested keeping the values in this file as defaults, which is the opposite of what this change is for. Retracting that part, revised feedback below.
Now that I understand the intent is to get internal endpoint references out of the public repo, the approach makes sense and I'm on board with it. A few follow-ups.
This only removes the value from the tip, not from the repo
git log -p pipelines/azure-pipelines.release.yml will still surface the old value to anyone who looks, so the reference remains publicly retrievable after this merges. That is not a reason to hold the PR, it is strictly an improvement over the status quo, but it does mean this change alone probably does not close the item out. Worth confirming whether history rewrite is in scope or whether the residual exposure is being accepted, so nobody assumes the file is fully clean.
Line 23 looks like it is in the same category
WindowsContainerImage: 'onebranch.azurecr.io/windows/ltsc2022/vse2022:latest'This is still in the file and is the same shape of reference as the one being moved. If it belongs to the same cleanup, it might be cheaper to handle it in this PR than in a follow-up. If it has been determined to be fine to leave public, a short comment saying so would save the next person from re-litigating it.
Please make sure the variable group is locked
TelemetryEventSource.cs is compiled into the signed, shipped wingetcreate binary, so the repo, branch, and path it comes from are build inputs to a signed artifact. Pinned in YAML those were PR-reviewed and version-controlled. As pipeline variables they are not, which is the right tradeoff here but worth compensating for:
- Set
settableAtQueueTime: falseso they cannot be overridden on an individual run. - Keep them in a variable group with restricted edit permissions rather than as plain pipeline variables.
That preserves the intent while keeping arbitrary content from being pointed at a signed release.
Undefined variables fail late and unclearly
Undefined macro variables are not an error in ADO. $(TelemetrySourceRepository) is passed through as that literal string, so if the variables are missing or misnamed, PkgESGitFetch fails with a confusing "repository not found" style error rather than something that points at the real cause.
The preceding DeleteFiles@1 step does mean this fails the build rather than silently shipping the stub, which is good. A small guard step before the fetch that errors out if any of the three are empty or still literal $(...) would turn a puzzling failure into an obvious one. Optional, but cheap.
Smaller items
- The new comment says the values are defined in the pipeline variables but does not say which variable group. A name there would help whoever debugs a fetch failure later.
- Has the release pipeline been run green against this branch? Since a missing or misnamed variable only shows up at build time, a successful run seems worth having before merge.
I also checked azure-pipelines.yml and azure-pipelines.loc.yml. Neither references the telemetry fetch, so nothing else needs updating alongside this.
Approving so this is not blocked. The variable group question and line 23 can be handled as follow-ups.
ranm-msft
left a comment
There was a problem hiding this comment.
Approving per my updated review above. The two follow-ups (locked variable group, and whether line 23 is in the same category) can be handled separately and should not block this.
📖 Description
🔗 References
🔍 Validation
✅ Checklist
📋 Issue Type
Microsoft Reviewers: Open in CodeFlow