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

Switch Dockerfile and host.json check when creating function app #4062

Merged
merged 2 commits into from
Mar 29, 2024

Conversation

motm32
Copy link
Contributor

@motm32 motm32 commented Mar 29, 2024

Switching the two checks since it is more likely that users will have a host.json in their project than a Dockerfile. This way we can abandon the check when we don't find a Dockerfile.

@motm32 motm32 requested a review from a team as a code owner March 29, 2024 16:50
context.dockerfilePath = (await findFiles(hostPath, dockerfileGlobPattern))[0].fsPath;
// check if host.json is in the same directory as the Dockerfile
if ((await findFiles(dockerfilePath, hostFileName)).length > 0) {
context.dockerfilePath = (await findFiles(dockerfilePath, hostFileName))[0].fsPath;
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't this be the hostFileName path? I think if findFiles returns a length > 0, we can set context.dockerFilePath to dockerfilePath (the variable above)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh that is an issue with the switcharoo I was saving just the host.json directory so the dockerfilepath just includes the directory not the full path. I'll fix that.

context.dockerfilePath = (await findFiles(hostPath, dockerfileGlobPattern))[0].fsPath;
// check if host.json is in the same directory as the Dockerfile
if ((await findFiles(dockerfilePath, hostFileName)).length > 0) {
context.dockerfilePath = (await findFiles(dockerfilePath, dockerfileGlobPattern))[0].fsPath;
Copy link
Member

Choose a reason for hiding this comment

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

Why can't you just set it as

Suggested change
context.dockerfilePath = (await findFiles(dockerfilePath, dockerfileGlobPattern))[0].fsPath;
context.dockerfilePath = dockerfilePath;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

dockerfile path above is set to the directory not the actualy path of the dockerfile. This is because we check the host.json path using the dockerfile directory we found. When we pass this context value to ACA it won't work with just the directory path. I can make two seperate constants and set one as the dockerfile but that seemed a little over kill, but since it seems confusing I can do that if you want.

Copy link
Member

Choose a reason for hiding this comment

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

Well, the main thing that I wanted to avoid is to have to do another findFiles call in here, but I guess it was always like that so whatever 🤷

@motm32 motm32 merged commit 38c2f49 into main Mar 29, 2024
2 checks passed
@motm32 motm32 deleted the meganmott/switcharoo branch March 29, 2024 20:36
@microsoft microsoft locked and limited conversation to collaborators May 14, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants