Subscriptions targeting Arcade updating eng/common

[dkurepa]

this shouldn't be happening, but it happened in https://github.com/dotnet/arcade/pull/16416/files#diff-72b8f8e899b94872c6ead31fd06ec109da15bcb9ad2af6e78103d6763a31c637

[adamzip]

It seems that this particular arcade->arcade subscription has a null value or empty string for "TargetDirectory", and is making the following boolean be true

Remote.cs, line 211

        // If arcadeItem is not null, we need to update eng/common for dependency flow subscriptions that don't target the repo root
        // We need the "." root check because we don't want to update the root eng/common in arcade -> arcade and vmr -> vmr (sdk) band scenarios
        var mayNeedArcadeUpdate = arcadeItem != null && (repoUri != arcadeItem.RepoUri || relativeDependencyBasePath.ToString() != ".");

Should this be modified to also exclude subscriptions that have a null / empty TargetDirectory?

[dkurepa]

All of the arcade->arcade subscriptions have this value as null/empty string

Should this be modified to also exclude subscriptions that have a null / empty TargetDirectory

no, normal dependency flow subscriptions also get arcade updates and need to have their eng/common updated.

I'd repro this and see what went wrong

[adamzip]

no, normal dependency flow subscriptions also get arcade updates and need to have their eng/common updated.

Then what is it that's supposed to stop arcade->arcade subscriptions from having their eng/common updated?

[dkurepa]

this part (repoUri != arcadeItem.RepoUri || relativeDependencyBasePath.ToString() != ".")

[adamzip]

Right, so that's what I'm talking about -
Currently, this line will make "mayNeedArcadeUpdate" be true whenever the relativeDependencyBasePath is different from "." (including empty strings and nulls) - that's wrong, isn't it?
For arcade->arcade subscriptions, mayNeedArcadeUpdate needs to resolve to false, even when directory is null / empty

[adamzip]

Let's make it clear. When does eng/common need to be updated? Let me know if I got it right:

• Whenever it's a codeflow subscription targeting subdirectories of the VMR (ie: not ".", not empty string, and not null)
• Whenever it's a dependency flow subscription whose target repo is anything other than arcade (and here, target directory shouldn't matter, right? or maybe it matters if it's a dependency flow into the VMR)

And of course both conditions should also include arcadeItem != null

Is that actually all? Or are there more cases?
I don't know about backflows, I guess they also need to be included somehow

[dkurepa]

• Forward flow subscriptions update eng/common like any other code file
• Backflow subscriptions update it as normal (the special thing here is that they flow VMRs arcade eng/common, not the root eng/common)
• Let's separate dependency flow subs into two categories, ones with specified target directory and without:
  • With target directory, these will mostly be the vmr -> vmr subs into the higher sdk bands, i think they don't want to root only, everything else should be updated
  • Without target directory should always update it unless it's an arcade-arcade subscription