feat: undeploy endpoints #14456
feat: undeploy endpoints #14456
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThis pull request introduces a comprehensive update to the Quartz library and related infrastructure in the Altinn Studio Designer project. The changes include adding a new GitHub Actions workflow to check Quartz library updates, updating the Quartz package version to 3.13.1, creating new PostgreSQL database tables for Quartz scheduling, and adding an initial implementation for an application undeployment endpoint. The workflow ensures that SQL migration files are consistent with the library update. Changes
Possibly related PRs
Suggested labels
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
CodeRabbit Configuration File (
|
github-actions
bot
added
area/app-deploy
mirkoSekulic
force-pushed
the
feature/undeploy-endpoint
branch
from
b9ce245 to
38baac0
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 7
🧹 Nitpick comments (2)
backend/src/Designer/Controllers/DeploymentsController.cs (1)
120-123: Implement the undeploy functionality.The current implementation throws
NotImplementedException. Please implement the actual undeployment logic or create a tracking issue for this task.Would you like me to help create a tracking issue for implementing the undeployment logic?
backend/src/Designer/Migrations/SqlScripts/QuartzTables/tables_postgres.sql (1)
15-158: Consider adding table partitioning forqrtz_fired_triggers.The table structure is correct and follows Quartz's schema. However, for large-scale deployments, the
qrtz_fired_triggerstable can grow significantly. Consider implementing table partitioning byfired_timeto improve query performance and maintenance.
🛑 Comments failed to post (7)
backend/src/Designer/Models/Dto/UndeployRequest.cs (1)
3-9: 🛠️ Refactor suggestion
Add validation attributes to ensure data integrity.
Consider adding validation attributes to ensure the
EnvironmentNameis not null or empty:public class UndeployRequest { /// <summary> /// Environment name in which the deployment should be undeployed /// </summary> + [Required(ErrorMessage = "Environment name is required")] + [StringLength(50, ErrorMessage = "Environment name cannot exceed 50 characters")] public string EnvironmentName { get; set; } }📝 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.public class UndeployRequest { /// <summary> /// Environment name in which the deployment should be undeployed /// </summary> [Required(ErrorMessage = "Environment name is required")] [StringLength(50, ErrorMessage = "Environment name cannot exceed 50 characters")] public string EnvironmentName { get; set; } }backend/packagegroups/NuGet.props (1)
52-52: 💡 Codebase verification
Fix invalid
fetch-depthvalue in Quartz update workflowThe workflow file has an invalid
fetch-depth: 0dvalue that would cause checkout failures. Additionally, the Git diff command implementation could be more robust.
.github/workflows/desiger-check-quartz-update.yml:
- Remove the 'd' suffix from
fetch-depth: 0d- Consider using a more robust diff command:
git diff -U0 HEAD^ HEAD -- backend/packagegroups/NuGet.props | grep '^[+].*Quartz.*Version'🔗 Analysis chain
Fix the Git diff command in the Quartz update check workflow.
The pipeline failure indicates an issue with the Git diff command. This needs to be addressed in the workflow file.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify if the Quartz package reference exists and is properly formatted rg -U 'PackageReference.*Quartz.*Version="[0-9]+\.[0-9]+\.[0-9]+"' backend/packagegroups/NuGet.propsLength of output: 160
Script:
#!/bin/bash # Find workflow files containing Quartz-related update checks fd -e yml -e yaml . .github/workflows/ -x rg -l -i 'quartz.*diff|diff.*quartz' {}Length of output: 135
Script:
#!/bin/bash # Display the content of the Quartz update check workflow cat .github/workflows/desiger-check-quartz-update.ymlLength of output: 2115
🧰 Tools
🪛 GitHub Actions: Dotnet format check
[error] Git diff command failed while checking for Quartz version updates, resulting in a non-zero exit code (1)
backend/src/Designer/Controllers/DeploymentsController.cs (1)
117-123: 🛠️ Refactor suggestion
Add XML documentation for the new endpoint.
Add XML documentation to describe the endpoint's purpose, parameters, and return type:
+ /// <summary> + /// Undeploys an application from the specified environment + /// </summary> + /// <param name="org">Organisation name</param> + /// <param name="app">Application name</param> + /// <param name="undeployRequest">Request containing the environment name</param> + /// <param name="cancellationToken">Cancellation token</param> + /// <returns>IActionResult indicating success or failure</returns> [HttpPost("undeploy")] [Authorize(Policy = AltinnPolicy.MustHaveGiteaDeployPermission)] public async Task<IActionResult> Undeploy(string org, string app, [FromBody] UndeployRequest undeployRequest, CancellationToken cancellationToken)📝 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./// <summary> /// Undeploys an application from the specified environment /// </summary> /// <param name="org">Organisation name</param> /// <param name="app">Application name</param> /// <param name="undeployRequest">Request containing the environment name</param> /// <param name="cancellationToken">Cancellation token</param> /// <returns>IActionResult indicating success or failure</returns> [HttpPost("undeploy")] [Authorize(Policy = AltinnPolicy.MustHaveGiteaDeployPermission)] public async Task<IActionResult> Undeploy(string org, string app, [FromBody] UndeployRequest undeployRequest, CancellationToken cancellationToken) { await Task.CompletedTask; throw new NotImplementedException(); }.github/workflows/desiger-check-quartz-update.yml (3)
20-22:
⚠️ Potential issueUpdate the checkout action version and fix the fetch-depth value.
The current version of
actions/checkoutis outdated, and there's a typo in thefetch-depthvalue:- uses: actions/checkout@v2 + uses: actions/checkout@v4 with: - fetch-depth: 0d + fetch-depth: 0📝 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.uses: actions/checkout@v4 with: fetch-depth: 0🧰 Tools
🪛 actionlint (1.7.4)
20-20: the runner of "actions/checkout@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
26-32:
⚠️ Potential issueFix shell script quoting and error handling.
Add proper quoting and error handling to prevent word splitting and ensure script reliability:
run: | + set -euo pipefail - diff=$(git diff --unified=0 backend/packagegroups/NuGet.props | grep + | grep Quartz | grep Version) + diff="$(git diff --unified=0 "backend/packagegroups/NuGet.props" | grep "+" | grep "Quartz" | grep "Version" || true)" if [ -n "$diff" ]; then - echo "is-updated=true" >> $GITHUB_OUTPUT + echo "is-updated=true" >> "$GITHUB_OUTPUT" else - echo "is-updated=false" >> $GITHUB_OUTPUT + echo "is-updated=false" >> "$GITHUB_OUTPUT" fi📝 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.run: | set -euo pipefail diff="$(git diff --unified=0 "backend/packagegroups/NuGet.props" | grep "+" | grep "Quartz" | grep "Version" || true)" if [ -n "$diff" ]; then echo "is-updated=true" >> "$GITHUB_OUTPUT" else echo "is-updated=false" >> "$GITHUB_OUTPUT" fi🧰 Tools
🪛 actionlint (1.7.4)
26-26: shellcheck reported issue in this script: SC2086:info:3:29: Double quote to prevent globbing and word splitting
(shellcheck)
26-26: shellcheck reported issue in this script: SC2086:info:5:30: Double quote to prevent globbing and word splitting
(shellcheck)
44-57: 🛠️ Refactor suggestion
Improve error handling and add descriptive error messages.
Add better error handling and descriptive messages for the SQL verification:
run: | + set -euo pipefail + download_url=$(curl https://api.github.com/repos/quartznet/quartznet/releases/tags/v${{ steps.get-quartz-version.outputs.quartz-version }} | \ jq -r '.assets[] | select(.name | contains("Quartz.NET-${{ steps.get-quartz-version.outputs.quartz-version }}.zip")) | .browser_download_url') + if [ -z "$download_url" ]; then + echo "Error: Failed to get download URL for Quartz.NET release" + exit 1 + fi + mkdir qdwn curl -L -o qdwn/quartz.zip $download_url && unzip qdwn/quartz.zip && rm qdwn/quartz.zip - diff=$(diff -u backend/src/Designer/Migrations/Quartz/tables_postgres.sql qdwn/database/tables/tables_postgres.sql) + if ! diff -u "backend/src/Designer/Migrations/Quartz/tables_postgres.sql" "qdwn/database/tables/tables_postgres.sql" > diff.txt; then + echo "Error: SQL tables are not in sync with Quartz.NET release" + cat diff.txt + exit 1 + fi - if [ -n "$diff" ]; then - exit 1; - else - echo "Tables file is up to date" - fi + echo "Tables file is up to date"📝 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.run: | set -euo pipefail download_url=$(curl https://api.github.com/repos/quartznet/quartznet/releases/tags/v${{ steps.get-quartz-version.outputs.quartz-version }} | \ jq -r '.assets[] | select(.name | contains("Quartz.NET-${{ steps.get-quartz-version.outputs.quartz-version }}.zip")) | .browser_download_url') if [ -z "$download_url" ]; then echo "Error: Failed to get download URL for Quartz.NET release" exit 1 fi mkdir qdwn curl -L -o qdwn/quartz.zip $download_url && unzip qdwn/quartz.zip && rm qdwn/quartz.zip if ! diff -u "backend/src/Designer/Migrations/Quartz/tables_postgres.sql" "qdwn/database/tables/tables_postgres.sql" > diff.txt; then echo "Error: SQL tables are not in sync with Quartz.NET release" cat diff.txt exit 1 fi echo "Tables file is up to date"🧰 Tools
🪛 actionlint (1.7.4)
44-44: shellcheck reported issue in this script: SC2086:info:5:28: Double quote to prevent globbing and word splitting
(shellcheck)
backend/src/Designer/Designer.csproj (1)
58-58: 💡 Codebase verification
Quartz implementation is missing
The Quartz package is referenced in the project file, but the required implementation is missing. You need to:
- Add Quartz services configuration in Program.cs
- Configure PostgreSQL job store
- Register job classes with dependency injection
Example configuration to add in
ConfigureServices:services.AddQuartz(q => { q.UsePersistentStore(s => { s.UsePostgres(); }); }); services.AddQuartzHostedService();🔗 Analysis chain
Verify Quartz configuration in startup code.
The Quartz package is properly referenced. However, ensure that:
- Quartz is properly configured in the startup code
- PostgreSQL job store is configured
- Job classes are registered with dependency injection
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for Quartz configuration in startup code rg -l "AddQuartz|UseQuartz" --type cs # Search for job store configuration ast-grep --pattern 'UsePostgres($$$)'Length of output: 79
Script:
#!/bin/bash # Find Program.cs and Startup.cs files fd "Program.cs|Startup.cs" --type f # Search for Quartz namespace usage rg "using.*Quartz" --type cs # Search for IJob implementations ast-grep --pattern 'class $_ : IJob' # Search for any Quartz-related configuration rg "services\..*Quartz" --type csLength of output: 219
Script:
#!/bin/bash # Check Program.cs content cat backend/src/Designer/Program.csLength of output: 14857
mirkoSekulic
force-pushed
the
feature/undeploy-endpoint
branch
from
88da021 to
d434869
Compare
mirkoSekulic
force-pushed
the
feature/undeploy-endpoint
branch
10 times, most recently
from
b9f580a to
321dacd
Compare
mirkoSekulic
force-pushed
the
feature/undeploy-endpoint
branch
from
321dacd to
a590794
Compare
Description
Related Issue(s)
Verification
Documentation
Summary by CodeRabbit
Release Notes
New Features
Infrastructure
Technical Improvements