Making sure you see #7023 (comment)

Making sure you see #7023 (comment)

How is the comment gone now? I did see it when the PR was open, but now that the PR is merged it looks like something deleted it.

Anyway, I'm now going for option 2, explain why the changes don't appy to other branches:

In short, this PR's change is build performance, not compliance, so we don't have a requirement to make the same change across all branches. An effort was made to make the same changes anyway, but it was turning into a big effort, so to avoid getting into a sunken cost fallacy situation, I'm giving up on the backporting. See below for details.

I started backporting to other branches, and I created these PRs;

• [7.0.x] Use 1ES OutputParentDirectory to reduce scanning tasks #7027
• [6.14.x] Use 1ES OutputParentDirectory to reduce scanning tasks #7028
• [6.12.x] Use 1ES OutputParentDirectory to reduce scanning tasks #7031
• [6.11.x] Use 1ES OutputParentDirectory to reduce scanning tasks #7032

The 7.0.x PR works, and I'll merge it when I get approvals on it. However, before I started backporting to 6.10.x and earlier, I checked 6.14.x's build, and saw it was failing because the generate-sbom.yml task does not write the sbom to the same location as 7.0.x and later, causing the output publishing to fail. 6.12 and 6.11 have the same issue, the sbom is not in the same location as 7.0 and later.

A little root cause analysis, so we can all learn from this, is as follows:

• The original sbom implementation in this repo was to copy the parts of the Arcade SDK related to sbom generation, and use it. This includes copying the readme instructing people not to modify the files in the directory. No validation was added to ensure that the sbom was being included successfully in the vsix.
• Years later we had a requirement to use 1ES PT, and in order to minimize the size of the 1ES PT onboarding PR, changes to how we generate and reference the sbom was done in an independent PR. There was a comment on the PR raising awareness of the readme saying not to modify the files, but a decision was made to change the eng/common yaml anyway. The PR description explains the error message that was being mitigated by changing the generate-sbom.yml file.
• NuGet.Client's dev branch migrated to 1ES PT, and as 1ES PT is a compliance requirement, it was backported to all other branches
• NuGet.Client's dev branch forked into release-6.14.x at this time, and dev was now NuGet version 7.0.x
• dnceng onboarded NuGet.Client repo onto maestro updates of eng/common and other related files. Someone modified NuGet's build yaml to tell generate-sbom.yml to stop publishing the artifact, in addition to overwriting the customization of the sbom path made in the earlier PR. Perhaps this is why the error from the previous PR's description did not happen. Otherwise, I don't know why the PR build succeeded, rather than failing with the same error message from the pre-1ES PT sbom PR. Part of the work that was done included migrating from lcl to xlf files for localization, which looks like a big effort, so wasn't backported to earlier branches.
• We didn't have automation validating that the sbom was being ingested properly, so we didn't notice that our vsix was missing the sbom until we tried to insert NuGet into VS and VS's automation caught the bug. It was promptly fixed, but since the issue was limited to 7.0, it wasn't backported to earlier branches. Automation to fail the build was when the sbom is missing was also added later. This also wasn't cherry-picked to earlier branches.

So, wrapping this all up, if the intention of asking PR authors to backport CI changes to all branches is to minimize differences between branches was to make future cherry-picks smooth, unfortunately it hasn't worked. The localization changes were huge, and the effort to make the same changes in older branches is too risky, and would take more effort than needing to manually resolve other CI changes which backporting from 7.0, so it's not worth backporting the loc changes.

Another option is to backport everything other than the loc changes (all the sbom related changes from the links in the list, and copying just the sbom related files from the arcade/maestro onboard PR). In theory it could mean one CI diff would work on all branches for versions 7.0 and above, and another diff could work on all branches 6.14 and below. Unfortunately, we're not in that situation. All the 6.x branches have the same sbom situation, and still 6.14 changes had conflicts when copying to 6.12. Once resolved, the 6.12 diff had conflicts with 6.11.

A lot effort alternative is to add a copy task to copy the sbom to the parent output directory, similar to how the nupkgs and VS15 directories are. But that would still be a difference between the 7.0 and 6.14 branches, which could lead to more cherry-pick conflicts when backporting future CI changes.

I believe we may be spending more time manually resolving conflicts when backporting minor CI improvements like this one, than we we gain the next time we have a compliance change that forces us to modify all supported branches. Therefore, while I'll merge the 7.0.x branch backport, I'm not going to do it for the older branches.

How is the comment gone now? I did see it when the PR was open, but now that the PR is merged it looks like something deleted it.

The only idea I have is that someone deleted it. There's no bot configuration to delete it for any reason, so perhaps it was intentionally or accidentally deleted?
Thanks for preemptively addressing our servicing branches!