-
Notifications
You must be signed in to change notification settings - Fork 81
Refactor Azure Functions integration and enhance project structure #45
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
base: main
Are you sure you want to change the base?
Conversation
- Updated Dockerfile to use Python 3.10 and install Azure Functions Core Tools. - Modified devcontainer.json for improved development environment setup. - Expanded .funcignore to exclude additional files and directories. - Enhanced README.md with detailed features, installation instructions, and security practices. - Implemented logging in function handlers and added new API routes. - Updated Bicep templates for better resource management and diagnostics. - Added security scan workflow for automated vulnerability checks. - Introduced Code of Conduct and Security Policy documents. - Updated requirements.txt with specific package versions for better dependency management.
This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation. |
4 similar comments
This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation. |
This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation. |
This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation. |
This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation. |
I like what this PR is aiming to do, but I'd ask we collab on a few implementation details:
@spboyer and @pamelafox could we collab and merge a rev of this with the points above handled? |
@@ -1,5 +1,5 @@ | |||
{ | |||
"name": "FastAPI on Azure Functions", | |||
"name": "Python FastAPI Function", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the original title was more specific/cleaer
] | ||
} | ||
}, | ||
"postCreateCommand": "python3 -m venv .venv", | ||
"postCreateCommand": "pip install -r requirements.txt", | ||
"postAttachCommand": ". .venv/bin/activate", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems weird that postAttachCommand activates venv, but postCreate no longer makes it? I think that's wrong. postCreate should still make it?
### Prerequisites | ||
- Python 3.9 or later | ||
- Azure Functions Core Tools | ||
- Azure CLI |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Azure CLI? Shouldnt it be azd? Or does it need both?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it should be azd only
@@ -0,0 +1,7 @@ | |||
from fastapi import FastAPI | |||
|
|||
app = FastAPI() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this file used?
@copilot please address my comments and Pamelafox's |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Refactors Azure Functions integration with FastAPI, enhances infrastructure templates, updates the development container, and expands project documentation and security practices.
- Reworked function_app entrypoint using
FunctionApp
and ASGI middleware, removed previous wrapper approach - Parameterized and improved Bicep templates (resource group logic, tags, role assignment, outputs)
- Updated devcontainer Dockerfile to Python 3.10, added security scan workflow, and expanded README/SECURITY docs
Reviewed Changes
Copilot reviewed 22 out of 24 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
startup.py | Added simple FastAPI test route |
infra/main.bicep | Parameterized resource group, tags, module renames, outputs |
infra/core/storage/storage-account.bicep | Added new storage account parameters and updated resource blocks |
infra/core/host/functions.bicep | Enabled managed identity, skip role assignment logic, added roleAssignment |
infra/core/host/appservice.bicep | Unified tags with azd-service-name |
host.json | Expanded logging config, bumped extensionBundle version, set timeout |
function_app.py | Defined FastAPI routes, created FunctionApp , applied ASGI middleware |
SECURITY.md | Added security policy template |
README.md | Expanded installation instructions, features, and security notice |
.devcontainer/Dockerfile | Switched base image to Python 3.10, venv creation, updated tools |
.github/workflows/security-scan.yml | Introduced weekly security scan workflow |
Comments suppressed due to low confidence (3)
SECURITY.md:8
- Replace the placeholder '[SECURITY CONTACT EMAIL]' with an actual security contact email address.
2. Send details of the vulnerability to [SECURITY CONTACT EMAIL].
README.md:91
- [nitpick] There's a second '## Prerequisites' section here. Consider consolidating duplicate sections to improve the README's clarity.
## Prerequisites
infra/main.bicep:18
- Using uniqueString(name) reduces the uniqueness domain compared to the original uniqueString(subscription().id, name, location), which may cause name collisions across subscriptions or regions. Consider reverting to the original inputs for collision safety.
var prefix = '${name}-${uniqueString(name)}'
@@ -2,86 +2,111 @@ targetScope = 'subscription' | |||
|
|||
@minLength(1) | |||
@maxLength(64) | |||
@description('Name which is used to generate a short unique hash for each resource') | |||
@description('Name of the the environment which is used to generate a short unique hash used in all resources.') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Duplicate word 'the the' in the description. It should read 'Name of the environment which is used to generate a short unique hash used in all resources.'
@description('Name of the the environment which is used to generate a short unique hash used in all resources.') | |
@description('Name of the environment which is used to generate a short unique hash used in all resources.') |
Copilot uses AI. Check for mistakes.
@@ -7,7 +7,7 @@ param tags object = {} | |||
param applicationInsightsName string = '' | |||
param appServicePlanId string | |||
param keyVaultName string = '' | |||
param managedIdentity bool = !empty(keyVaultName) | |||
param managedIdentity bool = true // Always enable managed identity |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The managedIdentity parameter is now always true and no longer drives any conditional logic. It can be removed to simplify this template.
param managedIdentity bool = true // Always enable managed identity |
Copilot uses AI. Check for mistakes.
from fastapi import FastAPI | ||
|
||
app = FastAPI() | ||
|
||
@app.get("/api/test") | ||
async def test(): | ||
return {"message": "FastAPI on Azure Functions"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] startup.py defines a FastAPI app that isn't referenced by the current function.json bindings. Consider removing or consolidating it with function_app.py to avoid dead code.
from fastapi import FastAPI | |
app = FastAPI() | |
@app.get("/api/test") | |
async def test(): | |
return {"message": "FastAPI on Azure Functions"} | |
# Removed unused FastAPI app and route definition as it is not referenced elsewhere. |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please see comments and resolve.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure this should be included.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should not be needed in pythonv2 model.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do not include this. We should use Pythonv2 programming model
|
||
## Costs | ||
The main cost components for this solution are: | ||
- Azure Functions consumption plan (pay-per-execution) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Azure Functions consumption plan (pay-per-execution) | |
- Azure Functions Flex Consumption plan (pay-per-execution) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We will not need this file anymore once we use the supported http streams and fastapi extension for azure functions. https://learn.microsoft.com/en-us/azure/azure-functions/functions-bindings-http-webhook-trigger?tabs=python-v2%2Cisolated-process%2Cnodejs-v4%2Cfunctionsv2&pivots=programming-language-python#example
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
File not needed in Python v2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
File not needed in Python v2
import azure.functions as func | ||
from fastapi import FastAPI |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to add extension per doc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really all of this bicep should be replaced by the newer AVM based version here:
https://github.com/Azure-Samples/functions-quickstart-python-http-azd/tree/main/infra
Refactor the integration of Azure Functions and improve the project structure. Update the Dockerfile for Python 3.10, enhance the development environment, and expand the README with installation and security practices. Implement logging in function handlers, add new API routes, and update Bicep templates for better resource management. Introduce a security scan workflow and add a Code of Conduct and Security Policy. Update dependencies for better management.