Skip to content

Conversation

bmanjwani
Copy link

Issue describing the changes in this PR

resolves #issue_for_this_pr

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
  • My changes should not be added to the release notes for the next release
    • Otherwise: I've added my notes to release_notes.md
  • I have added all required tests (Unit tests, E2E tests)

Additional information

Additional PR information

if (dockerImage == Constants.DockerImages.LinuxPython313ImageAmd64)
{
// creating temp folder for Dockerfile
string tempDir = Path.Combine(Path.GetTempPath(), "python313-docker");

Choose a reason for hiding this comment

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

lets name folder python313-buildenv-docker?

@@ -500,8 +500,20 @@ private static async Task RestorePythonRequirementsDocker(string functionAppRoot
dockerImage = await ChoosePythonBuildEnvImage();
Copy link
Member

Choose a reason for hiding this comment

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

I feel like this (ChoosePythonBuildEnvImage) should handle returning the local docker file name.

public const string LinuxPython313ImageAmd64 = "mcr.microsoft.com/azure-functions/python:4-python3.13-buildenv";

Can be replaced with something like local/python3.13-buildenv

ChoosePythonBuildEnvImage can be updated to return a tuple: imageName, isLocal

Then here below on line 503, we can check ifLocal = true and handle the image that way - this makes it more generic and not just for 313

Copy link
Member

Choose a reason for hiding this comment

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

This is just a quick thought - feel free to clean this up for something that makes more sense but the change should ensure we make these changes easy to expand on in future

if (dockerImage == Constants.DockerImages.LinuxPython313ImageAmd64)
{
// creating temp folder for Dockerfile
string tempDir = Path.Combine(Path.GetTempPath(), "python313-docker");
Copy link
Member

Choose a reason for hiding this comment

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

Code in this block and be broken out into its own method i.e. BuildLocalDockerImage(imageName)

And find a way to connect image name to the static resource.

Code in here should be generic for any local build docker image and not just for DockerfilePython313buildenv

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.

3 participants