Allow 'Add project reference' for existing projects

[david-acm]

Fix #40132

• Enable the PostActionProcessor to find existing project files
• Add unit test pr the PostActionProcessor
• Add IntegrationTest for dotnet new
• Extract new method for parsing 'targetFiles' as json

[david-acm]

@dotnet-policy-service agree

[baronfel]

Thanks for this - the need seems reasonable and the implementation looks sound. Thanks for adding tests. I'd like one of @joeloff / @MiYanni to look at this as well as myself, and ccing @phenning and @sayedihashimi for visibility, as PostAction changes need to be implemented separately for each 'host'.

In addition to this code work, there are a few follow-up efforts that I'd like to request be done:

• if there net-new property keys in the PostAction schema, we need to update the schemastore schema for template config files
• docs for the 'add a reference to a project file' PostAction need to be updated here

[phenning]

How do targetFiles get set here in a real template? It looks like they would always have this hardcoded path, or would it get passed in via a symbol somehow.

Note that the VS post action does not currently support targetFiles at all, and I would still have concerns about the user experience in that case for specifying the project as well.

One could always use the script execution post action to invoke dotnet add (currentProject) reference 'existingproject\existingproject.csproj' in this case as well.

[baronfel]

One could always use the script execution post action to invoke dotnet add (currentProject) reference 'existingproject\existingproject.csproj' in this case as well.

This is true, but I wanted to note that the script execution postAction is frequently cited as a security problem by the .NET security folks, and they have expressed a preference for more controlled/scenario-based extensibility instead of relying on the script action.

[phenning]

One could always use the script execution post action to invoke dotnet add (currentProject) reference 'existingproject\existingproject.csproj' in this case as well.

This is true, but I wanted to note that the script execution postAction is frequently cited as a security problem by the .NET security folks, and they have expressed a preference for more controlled/scenario-based extensibility instead of relying on the script action.

Also true, and also, that wouldn't work in Visual Studio anyway, due to lack of the script post action.

[david-acm]

How do targetFiles get set here in a real template? It looks like they would always have this hardcoded path, or would it get passed in via a symbol somehow.

Note that the VS post action does not currently support targetFiles at all, and I would still have concerns about the user experience in that case for specifying the project as well.

One could always use the script execution post action to invoke dotnet add (currentProject) reference 'existingproject\existingproject.csproj' in this case as well.

@phenning I only considered the use of the template from the cli, not Visual Studio. However:

• For a template author: the user experience remains unchanged, i.e. the targetFiles property will still be set from the template.json. The only difference is that now the templating engine will find an existing .*proj file. You can see an example usage in this template file I used for an integration test

• For a template user: I am not sure if symbol replacement works, but "sourceName" replacement does. The ExistingProject/ExistingProject.csproj is replaced by the template engine using "sourceName" and "applyFileRenamesToArgs":

{
  "$schema": "http://json.schemastore.org/template",
  ...
  "postActions": [
    {
      "description": "Adding a reference to another project",\
      "sourceName": "ExistingProject",
      "applyFileRenamesToArgs": [
        "targetFiles",
        "reference"
      ],
      "args": {
        "targetFiles": [
          "ExistingProject/ExistingProject.csproj"
        ]
      }
    }
  ]
}

I can add an integration test to tests this kind of renaming if needed

[phenning]

I don't think this solves the problem I mention. I don't see how when you invoke the template you are passing the actual name of the existing project to the new template.

If you have the following existing to which you want the post action to act on
src\AlreadyExists\AlreadyExists.csproj

and you create a template here even with the following post action, how does the actual path to the above get passed to the template when you instantiate it.

dotnet new TestAssets.PostActions.AddProjectReference.Existing

in this case would just try to look for "ExistingProject/ExistingProject.csproj", no? Or are you suggesting that the new template would look for a SPECIFIC named existing project?

@joeloff and @MiYanni should weigh in here as well, in case I'm missing something.

  "postActions": [
    {
      "Description": "Add ProjectReference to ExistingProject/ExistingProject.csproj",
      "ActionId": "B17581D1-C5C9-4489-8F0A-004BE667B814",
      "ContinueOnError": "false",
      "ManualInstructions": [
        {
          "Text": "Manually add the reference to Project1 in ExistingProject"
        }
      ],
      "args": {
        "targetFiles": [
          "ExistingProject/ExistingProject.csproj"
        ],
        "referenceType": "project",
        "reference": "Project1/Project1.csproj",
        "projectFileExtensions": ".csproj"
      }
    }
  ]

[david-acm]

If you have the following existing to which you want the post action to act on
src\AlreadyExists\AlreadyExists.csproj
and you create a template here even with the following post action, how does the actual path to the above get passed to the template when you instantiate it.
dotnet new TestAssets.PostActions.AddProjectReference.Existing

in this case would just try to look for "ExistingProject/ExistingProject.csproj", no? Or are you suggesting that the new template would look for a SPECIFIC named existing project?

@phenning No, as you suggested, the project file you want the post action to act on can be specified as a symbol, as long as the symbol definition specifies a"fileRename"that matches the placeholder in "targetFiles". I just confirmed that this does work, and I'll be adding an integration test to test/document the use case.

This is what the symbol definition and post action would look like:

{
  "symbols": {
    "existingProject": {
      ...
      "type": "parameter",
      "datatype": "string",
      "defaultValue": "ExistingProject/ExistingProject.csproj",
      "fileRename": "ExistingProjectPath" // 👈 Must be same as targetFile
    }
  },
  "postActions": [
    {
      "Description": "Add ProjectReference to ExistingProject/ExistingProject.csproj",
      "applyFileRenamesToArgs": [
        "targetFiles" // ❗️Must be specified
      ],
      "args": {
        "targetFiles": [
          "ExistingProjectPath" // 👈 Must be same as fileRename
        ],
        "referenceType": "project",
        "reference": "Project1/Project1.csproj"
      }
    }]
}

And this would be the instantiation command:

dotnet new TestAssets.PostActions.AddProjectReference.Existing --existingProject src/AlreadyExisting/AlreadyExisting.csproj

Hope this solves the problem you stated before. Let me know if you have further questions

[david-acm]

It took me some time but now all integration and backwards compatibility tests are passing. Can I get a check? @dotnet/domestic-cat

Thanks in advance.

[MiYanni]

👍

[Banner-Keith]

Who needs to complete this PR?

[david-acm]

Hi team, can you provide an ETA on merging this PR?

[baronfel]

Chatted with @phenning again and we're good to merge this one. @david-acm can you update the PostActionRegistry when you get a moment with docs for the new change?

[baronfel]

Once we figure out how to clear the pending checks we'll merge this.

[ellahathaway]

Once we figure out how to clear the pending checks we'll merge this.

The pending checks require pipeline ymls that were added into sdk pretty recently (definitely after the last time this PR was updated). Try merging the main branch into this PR to see if that queues the checks.

[ViktorHofer]

I see that the last time this PR run was three weeks ago. Make sure to merge with upstream to have fresh results. That will also make the new checks run.

[marcpopMSFT]

I see that the last time this PR run was three weeks ago. Make sure to merge with upstream to have fresh results. That will also make the new checks run.

I'll think on it. I understand the reasoning but have also been frustrated by this requirement elsewhere.

[david-acm]

@baronfel you can find the updated documentation at the dotnet/templating repo PR #8919.

[Banner-Keith]

@baronfel Looks like all the checks are passing now. Could we get this through? I am excited to get to use this new feature that david-acm has worked on.

[baronfel]

Yep! Sorry for the delay here, been juggling a few things and I let this one slip through the cracks.

[baronfel]

Thank you @david-acm for contributing the new feature and docs to go alongside it!

[Banner-Keith]

Thanks @baronfel and sorry for harassing you about it. 😄

[baronfel]

Your 'harassing' was very much appreciated :) I probably couldn't do my job without GitHub notifications!