Skip to content

Move telemetry source configuration to pipeline variables - #698

Merged
AmirMS (AmelBawa-msft) merged 1 commit into
mainfrom
user/amelbawa/release-var
Aug 29, 2026
Merged

Move telemetry source configuration to pipeline variables#698
AmirMS (AmelBawa-msft) merged 1 commit into
mainfrom
user/amelbawa/release-var

Conversation

@AmelBawa-msft

@AmelBawa-msft AmirMS (AmelBawa-msft) commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

📖 Description

🔗 References

🔍 Validation

✅ Checklist

📋 Issue Type

  • Bug fix
  • Feature
  • Task
Microsoft Reviewers: Open in CodeFlow

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
There may be pipelines that require an authorized user to comment /azp run to run.

@ranm-msft ranm-msft left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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: false so 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 ranm-msft left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.

@AmelBawa-msft
AmirMS (AmelBawa-msft) merged commit bcd01e5 into main Aug 29, 2026
14 of 16 checks passed
@AmelBawa-msft
AmirMS (AmelBawa-msft) deleted the user/amelbawa/release-var branch August 29, 2026 03:44
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.

2 participants