feat(service): add permissions for service-bus #1305
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThis pull request introduces several changes to the Azure Bicep files and CI/CD workflows. A new parameter Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (13)
.azure/applications/service/prod.bicepparam (2)
10-10: LGTM: Addition ofserviceBusNamespaceNameparameterThe new parameter for Service Bus Namespace is correctly implemented using the
readEnvironmentVariablefunction. This addition directly supports the PR objective of adding permissions for service-bus.Consider adding a brief comment above this parameter to explain its purpose and relationship to the service-bus permissions being added in this PR. This would enhance the file's self-documentation.
11-11: LGTM: Repositioning of secrets commentThe movement of the "// secrets" comment is appropriate given the reorganization of parameters. It now correctly precedes the
appInsightConnectionStringparameter.Consider using a more prominent comment style for better visibility, such as:
// ===== Secrets =====This would make the section division clearer and improve the overall structure of the file.
.azure/applications/service/test.bicepparam (2)
10-10: LGTM: New parameter for Service Bus namespace added.The
serviceBusNamespaceNameparameter is correctly declared and aligns with the PR objective. Good job on using an environment variable for configuration.Consider adding a brief comment above this parameter to explain its purpose, similar to how you've added comments for other sections (e.g., "// secrets").
12-13: LGTM: App Insights connection string parameter added.The addition of the
appInsightConnectionStringparameter is a good improvement for monitoring capabilities. The "// secrets" comment enhances code organization.For consistency, consider moving the
appConfigurationNameparameter under the "// secrets" comment if it contains sensitive information. If not, you might want to add a comment above the non-secret parameters for clarity..azure/applications/service/staging.bicepparam (1)
10-10: Approve addition ofserviceBusNamespaceNameparameter.The addition of the
serviceBusNamespaceNameparameter aligns with the PR objective of adding permissions for service-bus. This parameter will allow specifying the Azure Service Bus Namespace, which is crucial for the service-bus functionality.Consider updating the project documentation to reflect this new parameter and its purpose in the service-bus permissions feature.
.github/workflows/dispatch-apps.yml (1)
Line range hint
17-21: Approve the addition of therunMigrationparameter with a minor suggestion.The new
runMigrationparameter is a valuable addition to the workflow, providing more control over the deployment process. It allows users to optionally run migrations, which can be crucial for certain deployments while unnecessary for others.Consider slightly modifying the description for clarity:
- description: "Whether to run migration or not" + description: "Run database migrations during deployment"This change makes the purpose of the parameter more explicit.
.github/workflows/ci-cd-pull-request-release-please.yml (1)
61-61: LGTM! Consider adding a comment for clarity.The addition of the
AZURE_SERVICE_BUS_NAMESPACE_NAMEsecret is correct and aligns with the PR objectives. This change enables the workflow to access the Azure Service Bus namespace during the deployment process.Consider adding a brief comment above this line to explain the purpose of this secret, similar to the existing comment for other secrets. For example:
# Secret for accessing the Azure Service Bus namespace AZURE_SERVICE_BUS_NAMESPACE_NAME: ${{ secrets.AZURE_SERVICE_BUS_NAMESPACE_NAME }}This would improve the readability and maintainability of the workflow file.
.github/workflows/ci-cd-staging.yml (2)
68-68: LGTM! Consider updating documentation.The addition of the
AZURE_SERVICE_BUS_NAMESPACE_NAMEsecret is consistent with the PR objectives of adding permissions for service-bus. This change allows the deployment process to access the Azure Service Bus namespace.Consider updating the project documentation to reflect this new secret requirement for the CI/CD process. This will help other developers understand the necessary configuration for the workflow.
Line range hint
73-73: LGTM! Consider adding a comment for clarity.The addition of the
runMigrationparameter is a good improvement to the workflow. It provides more granular control over when database migrations are run during the deployment process, enhancing efficiency by avoiding unnecessary migrations.Consider adding a brief comment above this line to explain the purpose of the
runMigrationparameter. For example:# Determine whether to run migrations based on the event type or detected changes runMigration: ${{ github.event_name == 'workflow_dispatch' || needs.check-for-changes.outputs.hasMigrationChanges == 'true' }}This will help other developers quickly understand the logic behind this parameter.
.github/workflows/ci-cd-main.yml (1)
Line range hint
1-180: Consider potential impacts and update PR descriptionThe addition of the
AZURE_SERVICE_BUS_NAMESPACE_NAMEsecret to thedeploy-appsjob is a good start for integrating Service Bus permissions. However, consider the following points:
Evaluate if any other jobs in the workflow might need access to the Service Bus namespace. If so, you may need to add the secret to those jobs as well.
Update the PR description to provide context for this change. Explain why the Service Bus namespace is needed and how it will be used in the deployment process. This will help reviewers and future maintainers understand the purpose of this addition.
Would you like assistance in drafting an update for the PR description?
.github/workflows/ci-cd-prod.yml (3)
76-76: LGTM! Consider updating documentation.The addition of the
AZURE_SERVICE_BUS_NAMESPACE_NAMEsecret is correct and aligns with the PR objective of adding permissions for service-bus. This will allow the workflow to access the Azure Service Bus namespace during the dry run deployment.Consider updating the project documentation to reflect this new secret requirement for the CI/CD process.
Line range hint
82-82: LGTM! Consider improving readability.The addition of the
runMigrationinput parameter is a good improvement. It allows for conditional execution of migrations based on the event type or detected changes, which can optimize the workflow.For improved readability, consider breaking the condition into multiple lines:
runMigration: > ${{ github.event_name == 'workflow_dispatch' || needs.check-for-changes.outputs.hasMigrationChanges == 'true' }}This multi-line format can make the condition easier to read and maintain, especially if more conditions are added in the future.
Line range hint
1-150: Overall assessment: Changes improve workflow functionality and align with PR objectives.The modifications to this CI/CD workflow file successfully incorporate the new Service Bus configuration and enhance control over database migrations. These changes are consistent across both the dry run and actual deployment jobs, maintaining the integrity of the workflow structure.
Key improvements:
- Addition of the
AZURE_SERVICE_BUS_NAMESPACE_NAMEsecret allows for proper configuration of the Azure Service Bus during deployments.- Introduction of the
runMigrationparameter provides fine-grained control over when database migrations are executed.These enhancements align well with the PR objective of adding permissions for service-bus and improve the overall flexibility of the deployment process.
As the workflow grows in complexity, consider:
- Breaking down large jobs into smaller, more focused jobs or reusable workflows for improved maintainability.
- Implementing a centralized secret management strategy to reduce the need for individual secret declarations in each job.
- Creating a separate workflow file for database migration tasks if they become more complex or numerous in the future.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (12)
- .azure/applications/service/main.bicep (3 hunks)
- .azure/applications/service/prod.bicepparam (1 hunks)
- .azure/applications/service/staging.bicepparam (1 hunks)
- .azure/applications/service/test.bicepparam (1 hunks)
- .azure/modules/serviceBus/addReaderRoles.bicep (1 hunks)
- .github/workflows/ci-cd-main.yml (1 hunks)
- .github/workflows/ci-cd-prod.yml (2 hunks)
- .github/workflows/ci-cd-pull-request-release-please.yml (1 hunks)
- .github/workflows/ci-cd-pull-request.yml (1 hunks)
- .github/workflows/ci-cd-staging.yml (1 hunks)
- .github/workflows/dispatch-apps.yml (1 hunks)
- .github/workflows/workflow-deploy-apps.yml (3 hunks)
🧰 Additional context used
🔇 Additional comments (23)
.azure/applications/service/prod.bicepparam (3)
8-8: LGTM: Addition ofappConfigurationNameparameterThe new parameter for App Configuration is correctly implemented using the
readEnvironmentVariablefunction. This addition aligns with the PR objective and follows good practices for handling configuration in Azure deployments.
9-9: LGTM: Reordering ofcontainerAppEnvironmentNameparameterThe movement of this parameter improves the organization of the file by grouping related parameters together. This change enhances readability without affecting functionality.
Line range hint
1-13: Overall assessment: Changes align with PR objectives and follow best practicesThe modifications to this file successfully introduce the necessary parameters for service-bus integration while maintaining good security practices through the use of environment variables. The reordering of parameters enhances readability.
To further improve the file:
- Consider adding brief comments for new parameters to explain their purpose.
- Enhance the visibility of section dividers (like the "secrets" comment) for better file structure.
These changes effectively support the PR's goal of adding permissions for service-bus.
.azure/applications/service/test.bicepparam (2)
8-8: LGTM: Parameter declaration looks good.The
appConfigurationNameparameter is correctly declared and uses the appropriate environment variable.
Line range hint
1-13: Overall, the changes look good and align with the PR objectives.The additions and modifications in this file are well-implemented and follow good practices for Azure Bicep parameter files. The new
serviceBusNamespaceNameparameter aligns with the PR objective of adding service-bus related configurations, and theappInsightConnectionStringparameter is a valuable addition for monitoring capabilities.To ensure consistency across different environment parameter files, please run the following verification script:
This script will compare the
test.bicepparamfile with other environment parameter files. Review any differences to ensure that the changes are applied consistently where appropriate.✅ Verification successful
Consistency across environment parameter files verified.
All environment parameter files are consistent with
.azure/applications/service/test.bicepparam. No discrepancies found.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check consistency of parameter declarations across environment files # Test: Verify that all environment parameter files have consistent declarations echo "Checking consistency across environment parameter files:" fd -e bicepparam -x diff -u .azure/applications/service/test.bicepparam {} \;Length of output: 205
.azure/applications/service/staging.bicepparam (3)
Line range hint
1-13: Overall approval with suggestions for documentation and security verification.The changes to this file are well-structured and align with the PR objectives. The use of environment variables for parameter values is consistent and follows good practices.
To ensure completeness:
- Update the project documentation to reflect these new parameters and their purposes.
- Verify that all new environment variables are properly set and secured in your CI/CD pipeline.
- Consider adding a brief comment in the PR description explaining the re-addition of the
appConfigurationNameparameter if it was intentionally removed before.
12-13: Approve addition ofappInsightConnectionStringparameter with a security reminder.The addition of the
appInsightConnectionStringparameter as a secret is appropriate for Application Insights integration. Marking it as a secret is a good security practice.Please ensure that the
AZURE_APP_INSIGHTS_CONNECTION_STRINGis properly set and secured in your CI/CD pipeline. To verify this, you can run the following script in your CI/CD environment:
8-8: Approve addition ofappConfigurationNameparameter, but clarification needed.The addition of the
appConfigurationNameparameter is appropriate for specifying the Azure App Configuration resource. However, the AI summary indicates that this parameter was previously removed and now re-added.Could you please clarify the reason for re-adding this parameter? Was it intentionally removed before, or is this correcting a previous oversight?
To verify the usage of this parameter, please run the following script:
.github/workflows/ci-cd-pull-request.yml (1)
85-85: Summary of changesThe addition of the
AZURE_SERVICE_BUS_NAMESPACE_NAMEsecret to thedry-run-deploy-appsjob is a well-implemented change that supports the PR objective of adding permissions for service-bus. The change is minimal and doesn't disrupt the existing workflow structure.To ensure a smooth integration:
- Verify the usage of this new secret in the
workflow-deploy-apps.ymlfile.- Check for consistency across other workflow files, documentation, and infrastructure templates.
- Update any related files if necessary to maintain project-wide consistency.
These steps will help ensure that the new Azure Service Bus functionality is properly integrated into the CI/CD process.
.azure/applications/service/main.bicep (2)
26-28: LGTM: New parameter for Service Bus namespace.The addition of the
serviceBusNamespaceNameparameter is well-defined and aligns with the PR objective of adding permissions for the service-bus. The description is clear, and the minimum length constraint is appropriate.
172-172: LGTM: Appropriate dependency added for Service Bus access policy.The addition of
serviceBusOwnerAccessPolicyto thedependsOnarray for thecontainerAppmodule is correct. This ensures that the Service Bus access policy is created before the container app, maintaining proper resource creation order..github/workflows/ci-cd-staging.yml (1)
Line range hint
1-134: Overall, the changes look good and align with the PR objectives.The modifications to the CI/CD workflow for the staging environment are well-implemented. They add support for Azure Service Bus integration and optimize the database migration process. These changes enhance the deployment process and are consistent with the goal of adding permissions for service-bus.
To further improve the workflow:
- Ensure that the
AZURE_SERVICE_BUS_NAMESPACE_NAMEsecret is properly set in the repository secrets.- Update the project documentation to reflect these new changes in the CI/CD process.
- Consider adding comments to explain the purpose of new parameters or complex conditions in the workflow file.
These suggestions will help maintain the clarity and maintainability of the CI/CD process as the project evolves.
.github/workflows/ci-cd-main.yml (1)
104-104: LGTM! Verify the secret in GitHub repository settings.The addition of the
AZURE_SERVICE_BUS_NAMESPACE_NAMEsecret is appropriate and aligns with the PR objective of adding permissions for service-bus. This secret will likely be used in the deployment process to configure the application's access to the Azure Service Bus.Please ensure that the
AZURE_SERVICE_BUS_NAMESPACE_NAMEsecret is properly set in the GitHub repository settings. You can verify this by running the following GitHub CLI command:If the secret is not listed, you'll need to add it to the repository secrets.
.github/workflows/ci-cd-prod.yml (1)
Line range hint
100-105: LGTM! Consider adding a verification step.The changes in the
deploy-appsjob correctly mirror those in thedry-run-deploy-appsjob. The addition of theAZURE_SERVICE_BUS_NAMESPACE_NAMEsecret and therunMigrationinput parameter maintains consistency between the dry run and actual deployment processes.To ensure that the
AZURE_SERVICE_BUS_NAMESPACE_NAMEsecret is properly set and that therunMigrationparameter is working as expected, consider adding a verification step. You can use the following script to check for the presence of the secret and the correct setting of therunMigrationparameter:This script will help ensure that the changes have been applied consistently and correctly across both the dry run and actual deployment jobs.
✅ Verification successful
Verification Scripts Contain Errors. Generating Corrected Scripts.
The initial verification scripts encountered errors due to incorrect regex syntax. To accurately verify the presence of the
AZURE_SERVICE_BUS_NAMESPACE_NAMEsecret and the correct setting of therunMigrationparameter in both jobs, please use the following corrected scripts:These corrected scripts adjust the regex patterns to properly match the desired lines without causing parse errors. They check for the presence of the
AZURE_SERVICE_BUS_NAMESPACE_NAMEsecret and therunMigrationparameter in both thedeploy-appsanddry-run-deploy-appsjobs within the specified YAML file.Please execute these scripts to verify the changes.
Shell Script Updated for Accurate Verification. Please Run the Following Scripts:
Please execute these updated scripts to verify the presence and correct configuration of the
AZURE_SERVICE_BUS_NAMESPACE_NAMEsecret and therunMigrationparameter in both thedeploy-appsanddry-run-deploy-appsjobs.
Verification Successful: Changes Approved
The
AZURE_SERVICE_BUS_NAMESPACE_NAMEsecret and therunMigrationparameter are correctly set in both thedeploy-appsanddry-run-deploy-appsjobs.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the presence of AZURE_SERVICE_BUS_NAMESPACE_NAME secret and correct setting of runMigration parameter # Test: Check for the presence of AZURE_SERVICE_BUS_NAMESPACE_NAME in both jobs echo "Checking for AZURE_SERVICE_BUS_NAMESPACE_NAME secret..." if rg --type yaml 'AZURE_SERVICE_BUS_NAMESPACE_NAME: \${{ secrets\.AZURE_SERVICE_BUS_NAMESPACE_NAME }}' .github/workflows/ci-cd-prod.yml | wc -l | grep -q 2; then echo "AZURE_SERVICE_BUS_NAMESPACE_NAME secret is present in both jobs." else echo "Error: AZURE_SERVICE_BUS_NAMESPACE_NAME secret is not present in both jobs or is incorrectly formatted." fi # Test: Check for the correct setting of runMigration parameter in both jobs echo "Checking runMigration parameter..." if rg --type yaml 'runMigration: \${{ github\.event_name == '\''workflow_dispatch'\'' \|\| needs\.check-for-changes\.outputs\.hasMigrationChanges == '\''true'\'' }}' .github/workflows/ci-cd-prod.yml | wc -l | grep -q 2; then echo "runMigration parameter is correctly set in both jobs." else echo "Error: runMigration parameter is not correctly set in both jobs or is incorrectly formatted." fiLength of output: 1421
Script:
#!/bin/bash # Description: Verify the presence of AZURE_SERVICE_BUS_NAMESPACE_NAME secret and correct setting of runMigration parameter # Test: Check for the presence of AZURE_SERVICE_BUS_NAMESPACE_NAME in both jobs echo "Checking for AZURE_SERVICE_BUS_NAMESPACE_NAME secret..." count=$(rg --type yaml 'AZURE_SERVICE_BUS_NAMESPACE_NAME:\s*\${{ secrets\.AZURE_SERVICE_BUS_NAMESPACE_NAME }}' .github/workflows/ci-cd-prod.yml | wc -l) if [ "$count" -ge 2 ]; then echo "AZURE_SERVICE_BUS_NAMESPACE_NAME secret is present in both jobs." else echo "Error: AZURE_SERVICE_BUS_NAMESPACE_NAME secret is not present in both jobs or is incorrectly formatted." fi # Test: Check for the correct setting of runMigration parameter in both jobs echo "Checking runMigration parameter..." count=$(rg --type yaml 'runMigration:\s*\${{ github\.event_name == '\''workflow_dispatch'\'' \|\| needs\.check-for-changes\.outputs\.hasMigrationChanges == '\''true'\'' }}' .github/workflows/ci-cd-prod.yml | wc -l) if [ "$count" -ge 2 ]; then echo "runMigration parameter is correctly set in both jobs." else echo "Error: runMigration parameter is not correctly set in both jobs or is incorrectly formatted." fiLength of output: 1469
Script:
#!/bin/bash # Description: Verify the presence of AZURE_SERVICE_BUS_NAMESPACE_NAME secret and correct setting of runMigration parameter # Test: Check for the presence of AZURE_SERVICE_BUS_NAMESPACE_NAME in both jobs echo "Checking for AZURE_SERVICE_BUS_NAMESPACE_NAME secret..." count=$(grep -F 'AZURE_SERVICE_BUS_NAMESPACE_NAME: ${{ secrets.AZURE_SERVICE_BUS_NAMESPACE_NAME }}' .github/workflows/ci-cd-prod.yml | wc -l) if [ "$count" -ge 2 ]; then echo "AZURE_SERVICE_BUS_NAMESPACE_NAME secret is present in both jobs." else echo "Error: AZURE_SERVICE_BUS_NAMESPACE_NAME secret is not present in both jobs or is incorrectly formatted." fi # Test: Check for the correct setting of runMigration parameter in both jobs echo "Checking runMigration parameter..." count=$(grep -F "runMigration: \${{ github.event_name == 'workflow_dispatch' || needs.check-for-changes.outputs.hasMigrationChanges == 'true' }}" .github/workflows/ci-cd-prod.yml | wc -l) if [ "$count" -ge 2 ]; then echo "runMigration parameter is correctly set in both jobs." else echo "Error: runMigration parameter is not correctly set in both jobs or is incorrectly formatted." fiLength of output: 842
.github/workflows/workflow-deploy-apps.yml (4)
23-24: LGTM: New secret for Azure Service Bus Namespace added.The addition of the
AZURE_SERVICE_BUS_NAMESPACE_NAMEsecret is consistent with the PR objective of adding permissions for service-bus. The secret is correctly marked as required, ensuring it's always provided when the workflow is called.
Line range hint
67-76: Verify: Is Service Bus Namespace required for migration job?The new
AZURE_SERVICE_BUS_NAMESPACE_NAMEsecret is not included in the environment variables for the migration job deployment steps. Please confirm if this omission is intentional or if the migration job requires access to the Service Bus Namespace.Also applies to: 95-104
180-180: LGTM: Service Bus Namespace secret correctly added to deployment steps.The
AZURE_SERVICE_BUS_NAMESPACE_NAMEsecret is properly added to both the dry-run and actual deployment steps for the applications. This ensures that the Service Bus Namespace information is available during the deployment process, which aligns with the PR objective of adding permissions for service-bus.Also applies to: 205-205
Line range hint
1-305: Summary: Service Bus Namespace integration looks good, with one point to verify.The changes to add the
AZURE_SERVICE_BUS_NAMESPACE_NAMEsecret are well-implemented and consistent with the PR objective. The secret is correctly added to the workflow call section and application deployment steps. However, please verify if the omission of this secret in the migration job steps is intentional or if it needs to be added there as well.Overall, the changes improve the workflow's capability to handle Service Bus related deployments, which aligns with the PR's goal of adding permissions for service-bus.
.azure/modules/serviceBus/addReaderRoles.bicep (5)
1-2: ParameterserviceBusNamespaceNameis well-definedThe parameter
serviceBusNamespaceNameis properly declared with a clear description, allowing for dynamic specification of the Service Bus namespace.
4-5: ParameterprincipalIdsis well-definedThe parameter
principalIdscorrectly accepts an array of principal IDs with a descriptive annotation, enabling assignment to multiple principals.
7-9: Correctly references existing Service Bus namespaceThe existing resource
serviceBusNamespaceis accurately referenced using the provided namespace name, ensuring that role assignments target the correct Service Bus instance.
12-15: Accurate reference to the Azure Service Bus Data Owner roleThe existing resource
serviceBusDataOwnerRoleDefinitionappropriately references the built-in Azure Service Bus Data Owner role using the correct scope (subscription()) and role definition ID.
17-27: Efficient role assignments using a loop constructThe resource
roleAssignmentefficiently creates role assignments for eachprincipalIdusing a loop. The scope, name, and properties are correctly configured, ensuring unique role assignments per principal.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (8)
README.md (7)
Line range hint
12-28: LGTM! Clear instructions for Podman installation on Mac.The new section provides comprehensive steps for installing Podman on Mac, which is crucial for local development. The instructions are clear and include necessary details such as installing dependencies and enabling privileged testcontainers.
Consider adding a brief explanation of why Podman is being used instead of Docker, as this might help developers understand the project's containerization strategy better.
Line range hint
30-46: LGTM! Comprehensive instructions for Podman setup on Windows.The new section provides detailed steps for installing Podman on Windows, including necessary prerequisites like WSL2 and Virtual Machine Platform. The instructions for setting up Podman Desktop and the Compose Extension are clear and helpful.
Consider adding a note about potential system restarts that might be required after installing WSL2 or enabling Virtual Machine Platform, as this is a common requirement for Windows users.
Line range hint
48-62: LGTM! Clear instructions for running the project with Podman.The updated section provides clear instructions for running the project using
podman compose. The addition of GUI services and their URLs is very helpful for developers. The note about the nginx proxy and scaling options is informative and useful.Consider adding a brief troubleshooting section or link to common issues that developers might encounter when running the project for the first time with Podman.
Line range hint
167-199: LGTM! Comprehensive guide for local development settings.The new section on local development settings is extremely helpful. It clearly explains how to toggle external resources and introduces the use of
appsettings.local.jsonfor personal configurations. The example of enabling debug logging locally is practical and illustrates the concept well.Consider adding a note about version controlling
appsettings.local.json. While it's mentioned that it's ignored by git, it might be helpful to explicitly state that developers should not commit this file to avoid accidentally sharing personal configurations.
Line range hint
224-258: LGTM! Comprehensive explanation of the deployment process.The updated section on the deployment process is excellent. It provides a clear, step-by-step explanation of how code moves from pull request to production, including the use of Release Please for version management and changelog generation. The inclusion of a visual workflow diagram is particularly helpful for understanding the process.
Consider adding a brief explanation of how hotfixes or emergency patches are handled in this deployment process, as this is a common scenario that developers might need to understand.
Line range hint
260-293: LGTM! Clear instructions for manual deployment.The new section on manual deployment using GitHub dispatch workflows is very helpful. It provides clear instructions on how to use
dispatch-apps.ymlanddispatch-infrastructure.yml, including explanations of the required inputs. The caution note is appropriate given the sensitive nature of manual deployments.Consider adding a note about logging or monitoring these manual deployments. It would be helpful to mention if there's a way to track who initiated a manual deployment and when, for auditing purposes.
Line range hint
350-360: LGTM! Clear instructions for deploying applications in a new environment.The new section provides concise steps for deploying applications in a new infrastructure environment. It covers the necessary GitHub secrets, parameter file creation, and the deployment process. The reference to the Common APIM Guide for exposing applications is helpful.
Consider adding a brief checklist or summary at the end of this section to help developers ensure they haven't missed any critical steps in the process of setting up a new environment.
.azure/modules/serviceBus/addDataOwnerRoles.bicep (1)
11-15: Update documentation link to remove locale specification.In the description of
serviceBusDataOwnerRoleDefinition, the link includes the localeen-us. Removing the locale makes the link neutral and avoids unnecessary redirects for users in different regions.Apply this diff to update the link:
-@description('This is the built-in Azure Service Bus Data Owner role. See https://learn.microsoft.com/en-us/azure/role-based-access-control/built-in-roles#azure-service-bus-data-owner') +@description('This is the built-in Azure Service Bus Data Owner role. See https://learn.microsoft.com/azure/role-based-access-control/built-in-roles#azure-service-bus-data-owner')
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- .azure/applications/service/main.bicep (3 hunks)
- .azure/modules/serviceBus/addDataOwnerRoles.bicep (1 hunks)
- README.md (1 hunks)
🧰 Additional context used
🔇 Additional comments (5)
.azure/applications/service/main.bicep (4)
26-28: LGTM: New parameter for Service Bus namespace.The addition of the
serviceBusNamespaceNameparameter is well-defined and aligns with the PR objective of adding permissions for service-bus. The parameter has appropriate constraints and a clear description.
143-149: Verify the level of permissions being assignedThe addition of the
serviceBusOwnerAccessPolicymodule is consistent with the PR objective. However, please confirm that assigning Data Owner roles is the intended level of access. If a more restrictive set of permissions would suffice (e.g., Data Sender or Data Receiver), consider adjusting the role assignment accordingly.
172-172: LGTM: Dependency on Service Bus access policyThe addition of
serviceBusOwnerAccessPolicyto thedependsOnarray is correct and necessary. This ensures that the Service Bus access policy is created before the container app, maintaining proper resource creation order.
24-24:⚠️ Potential issueSecurity concern: Removal of
@secure()decoratorThe
@secure()decorator has been removed from thecontainerAppEnvironmentNameparameter. This change could potentially expose sensitive information. Please provide clarification on why this change was made and confirm that it doesn't introduce any security risks..azure/modules/serviceBus/addDataOwnerRoles.bicep (1)
17-27: The role assignment logic is well-implemented.The use of a
forloop to iterate overprincipalIdsand assign roles is efficient and aligns with best practices in Bicep. Theguidfunction ensures that each role assignment has a unique and consistent name.
| resource serviceBusNamespace 'Microsoft.ServiceBus/namespaces@2022-10-01-preview' existing = { | ||
| name: serviceBusNamespaceName | ||
| } |
There was a problem hiding this comment.
Consider using a stable API version for the Service Bus namespace resource.
The resource Microsoft.ServiceBus/namespaces is using the preview API version '2022-10-01-preview'. For production environments, it's recommended to use a stable API version to ensure reliability and long-term support.
Apply this diff to update the API version:
-resource serviceBusNamespace 'Microsoft.ServiceBus/namespaces@2022-10-01-preview' existing = {
+resource serviceBusNamespace 'Microsoft.ServiceBus/namespaces@2021-06-01' existing = {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| resource serviceBusNamespace 'Microsoft.ServiceBus/namespaces@2022-10-01-preview' existing = { | |
| name: serviceBusNamespaceName | |
| } | |
| resource serviceBusNamespace 'Microsoft.ServiceBus/namespaces@2021-06-01' existing = { | |
| name: serviceBusNamespaceName | |
| } |
🤖 I have created a release *beep* *boop* --- ## [1.25.0](v1.24.0...v1.25.0) (2024-10-17) ### Features * **applications:** add scalers for cpu and memory ([#1295](#1295)) ([eb0f19b](eb0f19b)) * **infrastructure:** create new yt01 app environment ([#1291](#1291)) ([1a1ccc0](1a1ccc0)) * **service:** add permissions for service-bus ([#1305](#1305)) ([7bf4177](7bf4177)) * **service:** deploy application in container apps ([#1303](#1303)) ([a309044](a309044)) ### Bug Fixes * **applications:** add missing property for scale configuration ([3ffb724](3ffb724)) * **applications:** use correct scale configuration ([#1311](#1311)) ([b8fb3cc](b8fb3cc)) * Fix ID-porten acr claim parsing ([#1299](#1299)) ([8b8862f](8b8862f)) * **service:** ensure default credentials work ([#1306](#1306)) ([b1e6a14](b1e6a14)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Description
Related Issue(s)
Verification
Documentation
docs-directory, Altinnpedia or a separate linked PR in altinn-studio-docs., if applicable)Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores