Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Initializing Dockerfile for .NET isolated project does not honor existing TFM #3800

Conversation

VineethReyya
Copy link
Contributor

@VineethReyya VineethReyya commented Aug 23, 2024

Issue describing the changes in this PR

resolves #3771
-> This PR checks the current worker runtime of function app is dotnet and gets the target framework from the directory of function app, locating to the project file(.csproj). later in the method used to configure the Docker file

Pull request checklist

  • My changes do not require documentation changes
    • Otherwise: Documentation issue linked to PR
  • My changes do not need to be backported to a previous version
    • Otherwise: Backport tracked by issue/PR #issue_or_pr
  • I have added all required tests (Unit tests, E2E tests)

…ting TFM (#3771)

-> Add Check for dotnet worker runtime and get the targetframework from the FunctionApp root directory.
@VineethReyya VineethReyya requested a review from a team as a code owner August 23, 2024 06:56
@VineethReyya VineethReyya changed the title Initializing Dockerfile for .NET isolated project does not honor exis… Initializing Dockerfile for .NET isolated project does not honor existing TFM Aug 23, 2024
@kshyju
Copy link
Member

kshyju commented Aug 27, 2024

Can we please add tests for this code path as well? I understand we don't have existing test for this action. but you should be able to create one similar to the StartHostActionTests and other action tests.

Get the Project Root Element from the ProjectFile Path and gets the value of Target Framework
@VineethReyya
Copy link
Contributor Author

VineethReyya commented Aug 28, 2024

Can we please add tests for this code path as well? I understand we don't have existing test for this action. but you should be able to create one similar to the StartHostActionTests and other action tests.

Hi @kshyju, I saw some tests are added in the InitTests.cs class (contains all cli test cases of Init action) for the Init action. Do I need to create a new class similar to StartHostActionTests to add tests or need to add it the InitTests.cs?

image

@VineethReyya VineethReyya reopened this Aug 28, 2024
@kshyju
Copy link
Member

kshyju commented Aug 29, 2024

Can we please add tests for this code path as well? I understand we don't have existing test for this action. but you should be able to create one similar to the StartHostActionTests and other action tests.

Hi @kshyju, I saw some tests are added in the InitTests.cs class (contains all cli test cases of Init action) for the Init action. Do I need to create a new class similar to StartHostActionTests to add tests or need to add it the InitTests.cs?

image

Oh okay, then you can add to the existing test file.

@VineethReyya
Copy link
Contributor Author

Can we please add tests for this code path as well? I understand we don't have existing test for this action. but you should be able to create one similar to the StartHostActionTests and other action tests.

Hi @kshyju, I saw some tests are added in the InitTests.cs class (contains all cli test cases of Init action) for the Init action. Do I need to create a new class similar to StartHostActionTests to add tests or need to add it the InitTests.cs?
image

Oh okay, then you can add to the existing test file.

Hi @kshyju added tests for my changes.

Copy link
Member

@kshyju kshyju left a comment

Choose a reason for hiding this comment

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

Please tweak DetermineTargetFramework method as needed and use that.

@VineethReyya
Copy link
Contributor Author

Please tweak DetermineTargetFramework method as needed and use that.

Hi @kshyju done changes in DetermineTargetFramework method by adding optional parameter (ProjectFileName) to it.

@VineethReyya
Copy link
Contributor Author

@microsoft-github-policy-service agree

@microsoft-github-policy-service agree

Copy link
Member

@kshyju kshyju left a comment

Choose a reason for hiding this comment

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

Few more minor comments.

src/Azure.Functions.Cli/Actions/LocalActions/InitAction.cs Outdated Show resolved Hide resolved
src/Azure.Functions.Cli/Helpers/DotnetHelpers.cs Outdated Show resolved Hide resolved
@VineethReyya
Copy link
Contributor Author

Hi @kshyju, review the changes for the latest commit

kshyju
kshyju previously approved these changes Sep 10, 2024
@VineethReyya VineethReyya merged commit 2c5b814 into v4.x Sep 12, 2024
9 checks passed
@VineethReyya VineethReyya deleted the 3771-initializing-dockerfile-for-net-isolated-project-does-not-honor-existing-tfm branch September 12, 2024 14:41
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.

Initializing Dockerfile for .NET isolated project does not honor existing TFM
3 participants