Release/1.2.1 #2746
Release/1.2.1 #2746
Conversation
…ojects/StreetCode into feature/issue-1681
add dockerfile.dbupdate
…ojects/StreetCode into feature/issue-1681
This reverts commit fbdfcb1.
Added a Trivy scan for Docker images after the build
WalkthroughThis update introduces a new Dockerfile and entrypoint script to containerize and run a .NET database update utility. The Jenkins pipeline is extended to build, scan, and deploy both backend and dbupdate images, with added security scanning and Discord notifications. The application startup code no longer triggers automatic database migrations or seeding. Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer
participant Jenkins as Jenkins Pipeline
participant Docker as Docker Registry
participant Discord as Discord Webhook
participant Server as Deployment Server
Dev->>Jenkins: Pushes code (with Dockerfile.dbupdate, entrypoint.sh)
Jenkins->>Jenkins: Build backend image
Jenkins->>Jenkins: Build dbupdate image
Jenkins->>Jenkins: Run Trivy scan on both images
Jenkins->>Docker: Push backend image
Jenkins->>Docker: Push dbupdate image
Jenkins->>Server: Deploy backend and dbupdate containers
Jenkins->>Discord: Send deployment status notification
Possibly related PRs
Poem
Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 4
🔭 Outside diff range comments (1)
Jenkinsfile (1)
374-379:⚠️ Potential issueAuthenticate GitHub CLI before release creation
Thegh release createcommand will fail without a prior login. Use the injectedGH_TOKEN:+ withCredentials([string(credentialsId: 'GH_TOKEN', variable: 'GH_TOKEN')]) { + sh 'echo "$GH_TOKEN" | gh auth login --with-token' + } sh "gh release create v${vers} --generate-notes --draft"This ensures the CLI has permissions to publish the draft release.
🧹 Nitpick comments (9)
Dockerfile.dbupdate (2)
11-11: Remove debuglsfrom build stage
TheRUN ls -la /tmp/ScriptsMigrationline is useful for debugging but should be removed (or gated by a build argument) in production to keep the image lean and avoid unnecessary layers.
19-19: Fix typo in comment
The comment reads# Publishishing application; please correct it to# Publishing applicationto maintain professionalism.entrypoint.sh (1)
4-16: Review diagnostic verbosity for production
You print directory listings, network info, and routing tables at startup. These are great for debugging but may clutter logs in routine runs. Consider conditioning them on a debug flag or removing them in the production build.Jenkinsfile (6)
3-5: Rename boolean flags for clarity and grammar
IS_DBUPDATE_IMAGE_BUILDEDandIS_DBUPDATE_IMAGE_PUSHcontain a typo (builded). For readability, rename toIS_DBUPDATE_IMAGE_BUILTandIS_DBUPDATE_IMAGE_PUSHED.
16-18: Remove or use the unusedGH_TOKENcredential
You injectGH_TOKENintoenvironmentbut never reference it in the pipeline. Either remove this line or leverage it for GitHub CLI authentication in the release stage.
144-162: Optimize Docker build context and caching
Building both images with the full repo context without a.dockerignoremay include unnecessary files and bust cache often. Recommend adding a.dockerignore(to exclude.git, docs, test data, etc.) and, if possible, running eachdocker buildin its project subfolder:- sh "docker build -f Dockerfile.dbupdate -t ${username}/dbupdate:${env.CODE_VERSION} ." + dir('Streetcode/DbUpdate') { + sh "docker build -f ../../Dockerfile.dbupdate -t ${username}/dbupdate:${env.CODE_VERSION} ." +}This improves caching and reduces context size.
168-189: Enforce failures on critical Trivy vulnerabilities
Right now you append|| true, which masks exit codes. If you want to gate the build on security findings, remove|| trueor collect scan summaries and fail explicitly when critical issues are found.
260-269: Rename non-descriptive stage name
The stage titled'WHAT IS THE NEXT STEP'isn’t self-documenting. Consider renaming to'Confirm Production Deployment'or similar to clarify its purpose.
445-467: Consolidate Discord webhook credential usage
You defineDISCORD_WEBHOOK_URLinenvironmentand again viawithCredentialsin your function. Choose one approach: either reference the env var directly in thecurlcall or centralize credential retrieval insendDiscordNotification. This will simplify maintenance.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (20)
Streetcode/DbUpdate/DbUpdate.csprojis excluded by!**/Streetcode/DbUpdate/**Streetcode/DbUpdate/Program.csis excluded by!**/Streetcode/DbUpdate/**Streetcode/DbUpdate/appsettings.IntegrationTests.jsonis excluded by!**/Streetcode/DbUpdate/**Streetcode/Streetcode.DAL/Persistence/ScriptsMigration/HealthCheck.sqlis excluded by!**/ScriptsMigration/**,!**/Streetcode/Streetcode.DAL/Persistence/ScriptsMigration/**Streetcode/Streetcode.DAL/Persistence/ScriptsMigration/Script_20230703154732_UpdatePartnerModel.sqlis excluded by!**/ScriptsMigration/**,!**/Streetcode/Streetcode.DAL/Persistence/ScriptsMigration/**Streetcode/Streetcode.DAL/Persistence/ScriptsMigration/Script_20230804120930_Change_StreetcodeCategoryContent_Text_to_10000_simb.sqlis excluded by!**/ScriptsMigration/**,!**/Streetcode/Streetcode.DAL/Persistence/ScriptsMigration/**Streetcode/Streetcode.DAL/Persistence/ScriptsMigration/Script_20230905201616_Jobs.sqlis excluded by!**/ScriptsMigration/**,!**/Streetcode/Streetcode.DAL/Persistence/ScriptsMigration/**Streetcode/Streetcode.DAL/Persistence/ScriptsMigration/Script_20231130202258_DatestringLength.sqlis excluded by!**/ScriptsMigration/**,!**/Streetcode/Streetcode.DAL/Persistence/ScriptsMigration/**Streetcode/Streetcode.DAL/Persistence/ScriptsMigration/Script_20240104113247_JobsDescriptionLength.sqlis excluded by!**/ScriptsMigration/**,!**/Streetcode/Streetcode.DAL/Persistence/ScriptsMigration/**Streetcode/Streetcode.DAL/Persistence/ScriptsMigration/Script_20240104204923_AddIdentityTables.sqlis excluded by!**/ScriptsMigration/**,!**/Streetcode/Streetcode.DAL/Persistence/ScriptsMigration/**Streetcode/Streetcode.DAL/Persistence/ScriptsMigration/Script_20240122161255_ForFansLength.sqlis excluded by!**/ScriptsMigration/**,!**/Streetcode/Streetcode.DAL/Persistence/ScriptsMigration/**Streetcode/Streetcode.DAL/Persistence/ScriptsMigration/Script_20240407205620_AddRefreshTokenToUser.sqlis excluded by!**/ScriptsMigration/**,!**/Streetcode/Streetcode.DAL/Persistence/ScriptsMigration/**Streetcode/Streetcode.DAL/Persistence/ScriptsMigration/Script_20240417150303_AudioConstraintChange.sqlis excluded by!**/ScriptsMigration/**,!**/Streetcode/Streetcode.DAL/Persistence/ScriptsMigration/**Streetcode/Streetcode.DAL/Persistence/ScriptsMigration/Script_20240428073027_FactIndexAdd.sqlis excluded by!**/ScriptsMigration/**,!**/Streetcode/Streetcode.DAL/Persistence/ScriptsMigration/**Streetcode/Streetcode.DAL/Persistence/ScriptsMigration/Script_20250118172930_ChangeConstraints.sqlis excluded by!**/ScriptsMigration/**,!**/Streetcode/Streetcode.DAL/Persistence/ScriptsMigration/**Streetcode/Streetcode.DAL/Persistence/ScriptsMigration/Script_20250124043754_UserUpdate.sqlis excluded by!**/ScriptsMigration/**,!**/Streetcode/Streetcode.DAL/Persistence/ScriptsMigration/**Streetcode/Streetcode.DAL/Persistence/ScriptsMigration/Script_20250204150151_AddFavourites.sqlis excluded by!**/ScriptsMigration/**,!**/Streetcode/Streetcode.DAL/Persistence/ScriptsMigration/**Streetcode/Streetcode.DAL/Persistence/ScriptsMigration/StreetcodeAuthor.sqlis excluded by!**/ScriptsMigration/**,!**/Streetcode/Streetcode.DAL/Persistence/ScriptsMigration/**Streetcode/Streetcode.DAL/Persistence/ScriptsMigration/UpdateUserNameFieldsLenght.sqlis excluded by!**/ScriptsMigration/**,!**/Streetcode/Streetcode.DAL/Persistence/ScriptsMigration/**Streetcode/Streetcode.DAL/Persistence/ScriptsMigration/ZTest.sqlis excluded by!**/ScriptsMigration/**,!**/Streetcode/Streetcode.DAL/Persistence/ScriptsMigration/**
📒 Files selected for processing (4)
Dockerfile.dbupdate(1 hunks)Jenkinsfile(9 hunks)Streetcode/Streetcode.WebApi/Program.cs(0 hunks)entrypoint.sh(1 hunks)
💤 Files with no reviewable changes (1)
- Streetcode/Streetcode.WebApi/Program.cs
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Jenkins
🔇 Additional comments (1)
Jenkinsfile (1)
216-224: Verifydbupdateservice is included indocker-compose.yml
You stop and prunedbupdatealong with other services, but ensure yourdocker-composefile defines adbupdateservice so it will be restarted onup -d. Otherwise it may be inadvertently omitted.
|
|
||
| CMD ["dotnet", "DbUpdate.dll"] No newline at end of file |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Integrate the entrypoint.sh script into the image
You’ve added a runtime helper script (entrypoint.sh) but the Dockerfile never copies or uses it. Consider adding:
COPY --from=build /tmp/ScriptsMigration /app/ScriptsMigration/
+COPY entrypoint.sh /app/entrypoint.sh
+RUN chmod +x /app/entrypoint.sh
- CMD ["dotnet", "DbUpdate.dll"]
+ENTRYPOINT ["/app/entrypoint.sh"]This ensures the diagnostics, env-validation, and logging logic in entrypoint.sh actually runs.
📝 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.
| CMD ["dotnet", "DbUpdate.dll"] | |
| COPY --from=build /tmp/ScriptsMigration /app/ScriptsMigration/ | |
| COPY entrypoint.sh /app/entrypoint.sh | |
| RUN chmod +x /app/entrypoint.sh | |
| ENTRYPOINT ["/app/entrypoint.sh"] |
| #!/bin/bash | ||
| set -e |
There was a problem hiding this comment.
Ensure pipeline fails if dotnet fails
With only set -e, a failing dotnet ... | tee ... won’t trigger exit because the pipeline return code is from tee. Add pipefail:
-#!/bin/bash
-set -e
+#!/bin/bash
+set -euo pipefailThis guarantees any subprocess error aborts the script.
| printenv | grep -i connection || echo " No connection string in environment" | ||
| printenv | grep -i sql || true |
There was a problem hiding this comment.
Prevent leaking sensitive connection strings
The line printenv | grep -i connection can expose full credentials to logs. Either remove it or mask values:
-printenv | grep -i connection || echo " No connection string in environment"
+printenv | grep -i 'ConnectionStrings__DefaultConnection' | sed -E 's/(=.*)/=***MASKED***/' || echo " No connection string in environment"This protects PII and secrets.
📝 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.
| printenv | grep -i connection || echo " No connection string in environment" | |
| printenv | grep -i sql || true | |
| # Protect default connection string by masking its value | |
| printenv | grep -i 'ConnectionStrings__DefaultConnection' | sed -E 's/(=.*)/=***MASKED***/' || echo " No connection string in environment" | |
| printenv | grep -i sql || true |
|
|
|
||
| success { | ||
| script { | ||
| sendDiscordNotification('SUCCESS', 'Deployment pipeline completed successfully.') |
There was a problem hiding this comment.
Will this be executed even if the last successfully executed stage is rollback?
|
|
||
| } | ||
|
|
||
| post { |
There was a problem hiding this comment.
If I'm not wrong we expected to have Notifications only when we deploy/rollback or do anything else with Stage deployment, that's why I'm not sure about keeping all these notifications here in 'post'
Please verify if this meets original ticket requirements
Streetcode/Streetcode.DAL/Persistence/ScriptsMigration/HealthCheck.sql
Outdated
Show resolved
Hide resolved
Streetcode/Streetcode.DAL/Persistence/ScriptsMigration/ZTest.sql
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Is this file still needed?
There was a problem hiding this comment.
this question needs research
LanchevychMaxym
requested review from
IceStorman,
ZhmudAnastasiia,
ihor-yatsyshyn and
yaroslav-vasylyshyn
deleted "RUN ls -la /tmp/ScriptsMigration" Co-authored-by: Maxym <[email protected]>
Switched from the 'feature/add-init-container' branch to 'main'
Fix REGEXP pattern for matching release branch names
removed "connection string"
…heck.sql deleted "HealthCheck.sql" file
deleted "ZTest.sql" file
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
Jenkinsfile (1)
144-147: Great job updating the regex for multi-digit versionsThe branch pattern now uses
\\d+\\.\\d+\\.\\d+, which correctly supports branches likerelease/10.12.1.
🧹 Nitpick comments (4)
Jenkinsfile (4)
16-17: Remove redundant credential loading or unify usage
DISCORD_WEBHOOK_URLis loaded in theenvironmentblock but then re-fetched insidesendDiscordNotificationwithwithCredentials. Likewise,GH_TOKENisn’t directly referenced elsewhere. To avoid confusion:
- Remove the
environment { DISCORD_WEBHOOK_URL }entry and keep credential loading in your helper.- Or use
env.DISCORD_WEBHOOK_URLinsendDiscordNotificationand drop the extrawithCredentials.
153-162: Consider adding--pullto Docker build commandsTo ensure you always build from the latest base images (picking up security patches), include
--pull:- sh "docker build -f Dockerfile -t ${env.DOCKER_USERNAME}/streetcode:${env.CODE_VERSION} ." + sh "docker build --pull -f Dockerfile -t ${env.DOCKER_USERNAME}/streetcode:${env.CODE_VERSION} ." - sh "docker build -f Dockerfile.dbupdate -t ${env.DOCKER_USERNAME}/dbupdate:${env.CODE_VERSION} ." + sh "docker build --pull -f Dockerfile.dbupdate -t ${env.DOCKER_USERNAME}/dbupdate:${env.CODE_VERSION} ."
281-289: Extract rollback steps into a shared functionThe rollback logic in the aborted hook closely mirrors the deploy steps. To reduce duplication and simplify maintenance, extract these into a reusable Groovy function, for example:
def rollbackStage(backendTag, frontendTag) { sh "export DOCKER_TAG_BACKEND=${backendTag}" sh "export DOCKER_TAG_FRONTEND=${frontendTag}" sh 'docker compose --env-file /etc/environment down' sh 'docker compose --env-file /etc/environment up -d' sendDiscordNotification('ABORTED', 'Deployment was rolled back.') }
218-242:Details
❓ Verification inconclusive
Verify state flag persistence & streamline teardown
The
whencondition relies on Groovy varsIS_IMAGE_PUSHandIS_DBUPDATE_IMAGE_PUSHwhich may not persist across separatescriptblocks. Please confirm that these flags reliably guard theDeploy Stage. Additionally, consider replacing the explicitdocker stop+ prune sequence with Docker Compose’s built-in commands:
🏁 Script executed:
docker compose --env-file /etc/environment down --remove-orphans docker compose --env-file /etc/environment up -dLength of output: 311
Validate flag persistence & verify Docker Compose teardown
Please ensure that your
IS_IMAGE_PUSHandIS_DBUPDATE_IMAGE_PUSHflags are declared at the pipeline level (e.g., inparametersor the top-levelenvironmentblock) so they reliably persist into thewhenexpression and across allscriptblocks. Then, confirm in your Jenkins environment that:
- The Deploy Stage only executes when both flags are true.
- Docker Compose is installed on your agent and the following commands cleanly stop and restart your services without errors:
docker compose --env-file /etc/environment down --remove-orphans docker compose --env-file /etc/environment up -dSuggested replacement for the explicit prune/stop sequence:
- sh 'docker image prune --force --filter "until=72h"' - sh 'docker system prune --force --filter "until=72h"' - sh """ - docker stop backend frontend nginx loki certbot dbupdate - """ + sh ''' + docker compose --env-file /etc/environment down --remove-orphans + docker compose --env-file /etc/environment up -d + '''Please manually verify these compose commands on your build agents to ensure they behave as expected.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Streetcode/DbUpdate/Program.csis excluded by!**/Streetcode/DbUpdate/**
📒 Files selected for processing (1)
Jenkinsfile(9 hunks)
🔇 Additional comments (1)
Jenkinsfile (1)
193-206: Pushing both backend and dbupdate imagesThe extension to push the
dbupdateimage alongside the backend is clear and well-scoped. Credentials are correctly scoped and used.
| def IS_DBUPDATE_IMAGE_BUILDED = false | ||
| def IS_IMAGE_PUSH = false | ||
| def IS_DBUPDATE_IMAGE_PUSH = false |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use correct and consistent naming and scoping for build flags
The variables IS_IMAGE_BUILDED and IS_DBUPDATE_IMAGE_BUILDED use non-standard past-tense “BUILDED” and are declared as plain Groovy vars, which can lead to scoping issues across script blocks. Consider:
- Renaming to
IS_IMAGE_BUILTandIS_DBUPDATE_IMAGE_BUILTfor correct grammar. - Promoting these flags into the
environmentblock (e.g.,environment { IS_IMAGE_BUILT = 'false' }) or storing state incurrentBuildto ensure persistence between stages.
| def sendDiscordNotification(status, message) { | ||
| withCredentials([string(credentialsId: 'WEBHOOK_URL', variable: 'DISCORD_WEBHOOK_URL')]) { | ||
| def jsonMessage = """ | ||
| { | ||
| "content": "$status: $message", | ||
| "embeds": [ | ||
| { | ||
| "title": "Deployment Status", | ||
| "fields": [ | ||
| {"name": "Environment", "value": "Stage", "inline": true}, | ||
| {"name": "Pipeline Name", "value": "$env.JOB_NAME", "inline": true}, | ||
| {"name": "Status", "value": "$status", "inline": true}, | ||
| {"name": "Deployment Tag", "value": "$env.CODE_VERSION", "inline": true}, | ||
| {"name": "Date and Time", "value": "${new Date().format('yyyy-MM-dd HH:mm:ss')}", "inline": true}, | ||
| {"name": "Pipeline Link", "value": "[Click here]($env.BUILD_URL)", "inline": true} | ||
| ] | ||
| } | ||
| ] | ||
| } | ||
| """ | ||
| sh """curl -X POST -H 'Content-Type: application/json' -d '$jsonMessage' "\$DISCORD_WEBHOOK_URL" """ | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Leverage a Pipeline HTTP step & JSON builder
Building JSON via multiline strings and shelling out with curl can introduce escaping errors and lacks error handling. Using the Jenkins httpRequest step with groovy.json.JsonOutput makes this more robust:
import groovy.json.JsonOutput
def payload = JsonOutput.toJson([
content: "$status: $message",
embeds: [ /* ... */ ]
])
httpRequest(
httpMode: 'POST',
contentType: 'APPLICATION_JSON',
requestBody: payload,
url: DISCORD_WEBHOOK_URL
)| script { | ||
| sendDiscordNotification('SUCCESS', 'Deployment pipeline completed successfully.') | ||
| } | ||
| } | ||
| failure { | ||
| script { | ||
| sendDiscordNotification('FAILED', 'Deployment pipeline failed.') | ||
| } | ||
| } | ||
| aborted { | ||
| script { | ||
| sendDiscordNotification('ABORTED', 'Deployment pipeline was aborted.') | ||
| } | ||
| } |
There was a problem hiding this comment.
Parameterize environment in notifications
The pipeline-level post always sets "Environment": "Stage" in the Discord embed, even for production flows. This hard-coding can lead to misleading alerts. Extend sendDiscordNotification to accept an environment argument or derive it dynamically:
sendDiscordNotification('SUCCESS', 'Pipeline completed', 'Production')| stage('Trivy Security Scan') { | ||
| when { | ||
| expression { IS_IMAGE_BUILDED == true && IS_DBUPDATE_IMAGE_BUILDED == true } | ||
| } | ||
| steps { | ||
| script { | ||
| def imagesToScan = [ | ||
| "${env.DOCKER_USERNAME}/streetcode:${env.CODE_VERSION}", | ||
| "${env.DOCKER_USERNAME}/dbupdate:${env.CODE_VERSION}" | ||
| ] | ||
| imagesToScan.each { image -> | ||
| echo "Running Trivy scan on ${image}" | ||
| // Run Trivy scan and display the output in the console log ( || true - don't fail on exit code) | ||
| sh """ | ||
| docker run --rm \ | ||
| -v /var/run/docker.sock:/var/run/docker.sock \ | ||
| aquasec/trivy image --no-progress --severity HIGH,CRITICAL --exit-code 1 ${image} || true | ||
| """ | ||
| } |
There was a problem hiding this comment.
Fail the build on critical vulnerabilities
The Trivy scan appends || true, which masks exit codes so the pipeline never fails even if HIGH/CRITICAL issues are found. To align with your security policy, remove || true or explicitly check the exit code and fail when non-zero:
- aquasec/trivy image --no-progress --severity HIGH,CRITICAL --exit-code 1 ${image} || true
+ aquasec/trivy image --no-progress --severity HIGH,CRITICAL --exit-code 1 ${image}📝 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.
| stage('Trivy Security Scan') { | |
| when { | |
| expression { IS_IMAGE_BUILDED == true && IS_DBUPDATE_IMAGE_BUILDED == true } | |
| } | |
| steps { | |
| script { | |
| def imagesToScan = [ | |
| "${env.DOCKER_USERNAME}/streetcode:${env.CODE_VERSION}", | |
| "${env.DOCKER_USERNAME}/dbupdate:${env.CODE_VERSION}" | |
| ] | |
| imagesToScan.each { image -> | |
| echo "Running Trivy scan on ${image}" | |
| // Run Trivy scan and display the output in the console log ( || true - don't fail on exit code) | |
| sh """ | |
| docker run --rm \ | |
| -v /var/run/docker.sock:/var/run/docker.sock \ | |
| aquasec/trivy image --no-progress --severity HIGH,CRITICAL --exit-code 1 ${image} || true | |
| """ | |
| } | |
| stage('Trivy Security Scan') { | |
| when { | |
| expression { IS_IMAGE_BUILDED == true && IS_DBUPDATE_IMAGE_BUILDED == true } | |
| } | |
| steps { | |
| script { | |
| def imagesToScan = [ | |
| "${env.DOCKER_USERNAME}/streetcode:${env.CODE_VERSION}", | |
| "${env.DOCKER_USERNAME}/dbupdate:${env.CODE_VERSION}" | |
| ] | |
| imagesToScan.each { image -> | |
| echo "Running Trivy scan on ${image}" | |
| // Run Trivy scan and display the output in the console log (fail pipeline on exit code) | |
| sh """ | |
| docker run --rm \ | |
| -v /var/run/docker.sock:/var/run/docker.sock \ | |
| aquasec/trivy image --no-progress --severity HIGH,CRITICAL --exit-code 1 ${image} | |
| """ | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
Jenkinsfile (1)
144-147: Enhance release branch regex
The regexrelease/\\d+\\.\\d+\\.\\d+now supports multi-digit version segments, aligning with prior feedback. This change is correct and improves version matching.
🧹 Nitpick comments (7)
Jenkinsfile (7)
3-5: Initialize dbupdate image tracking flags
You've addedIS_DBUPDATE_IMAGE_BUILDEDandIS_DBUPDATE_IMAGE_PUSHto track the dbupdate image lifecycle. Consider using consistent naming (e.g.,IS_DBUPDATE_IMAGE_BUILT) and leveraging Jenkinsenv.scope for clarity.
16-17: Simplify credential injection
You loadDISCORD_WEBHOOK_URLinenvironmentand again insidesendDiscordNotification. You can remove the innerwithCredentialsand reference the env var directly. Also,GH_TOKENis injected but unused—remove it or wire it into a GitHub step.
154-161: Build backend and dbupdate images together
Grouping the two Docker builds under shared credentials is effective. To improve:
- Log each build’s exit code or output for easier debugging.
- Confirm
Dockerfile.dbupdatecontext includes all required files.
193-195: Gate image push on build success
The push stage checks both build flags correctly. Minor nit: renameIS_IMAGE_BUILDEDtoIS_IMAGE_BUILTfor grammatical consistency.
200-201: Use Jenkins Docker credential binding
Instead of pipingecho $password | docker login, considerdocker.withRegistryor the Jenkins Docker plugin’s credential binding for cleaner credential handling.
249-250: Parameterize Stage notification
Integrating Discord notifications is excellent for visibility. To avoid hard-coding, parameterize the environment name (e.g.,${env.DEPLOY_ENV}) instead of"Stage"in the embed.
262-262: Clarify stage title
WHAT IS THE NEXT STEPis ambiguous. Rename to something likeApprove Production Deploymentto improve readability and align with pipeline conventions.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Streetcode/DbUpdate/Program.csis excluded by!**/Streetcode/DbUpdate/**
📒 Files selected for processing (1)
Jenkinsfile(9 hunks)
🔇 Additional comments (3)
Jenkinsfile (3)
174-178: Optimize image list for scanning
DefiningimagesToScanas a list makes it straightforward to extend if more images are added. This is a clean, scalable approach.
205-207: Track dbupdate image push
SettingIS_DBUPDATE_IMAGE_PUSHafter pushing ensures downstream stages only run when both images are available. This is implemented correctly.
218-219: Enforce both images before deployment
Using both push flags to guard theDeploy Stageensures consistency across services. This conditional check is well-placed.
| stage('Trivy Security Scan') { | ||
| when { | ||
| expression { IS_IMAGE_BUILDED == true && IS_DBUPDATE_IMAGE_BUILDED == true } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Condition Trivy scan on both images
You guard the security scan with both build flags. To enforce security best practices, consider failing the pipeline on HIGH/CRITICAL findings (i.e., remove || true) or aggregate scan reports for review.
| sh """ | ||
| docker run --rm \ | ||
| -v /var/run/docker.sock:/var/run/docker.sock \ | ||
| aquasec/trivy image --no-progress --severity HIGH,CRITICAL --exit-code 1 ${image} || true | ||
| """ |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Strengthen vulnerability scan error handling
Masking failures with || true hides critical issues. Consider:
- Removing
|| trueso the stage fails on severity breaches. - Archiving Trivy JSON output as an artifact.
| def sendDiscordNotification(status, message) { | ||
| withCredentials([string(credentialsId: 'WEBHOOK_URL', variable: 'DISCORD_WEBHOOK_URL')]) { | ||
| def jsonMessage = """ | ||
| { | ||
| "content": "$status: $message", | ||
| "embeds": [ | ||
| { | ||
| "title": "Deployment Status", | ||
| "fields": [ | ||
| {"name": "Environment", "value": "Stage", "inline": true}, | ||
| {"name": "Pipeline Name", "value": "$env.JOB_NAME", "inline": true}, | ||
| {"name": "Status", "value": "$status", "inline": true}, | ||
| {"name": "Deployment Tag", "value": "$env.CODE_VERSION", "inline": true}, | ||
| {"name": "Date and Time", "value": "${new Date().format('yyyy-MM-dd HH:mm:ss')}", "inline": true}, | ||
| {"name": "Pipeline Link", "value": "[Click here]($env.BUILD_URL)", "inline": true} | ||
| ] | ||
| } | ||
| ] | ||
| } | ||
| """ | ||
| sh """curl -X POST -H 'Content-Type: application/json' -d '$jsonMessage' "\$DISCORD_WEBHOOK_URL" """ | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consolidate Discord notification logic
The sendDiscordNotification function is comprehensive, but you reload DISCORD_WEBHOOK_URL inside it despite already injecting it in environment. Consolidate credential handling to one location and ensure the triple‐quoted JSON payload handles quoting correctly (or switch to single‐quote style).
updated the Discord notifications logic to execute only when the status update about the deployment on the STG environment is available
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
Jenkinsfile (2)
3-5: 🛠️ Refactor suggestionInconsistent variable naming and scoping for build/push flags
The new flagsIS_DBUPDATE_IMAGE_BUILDEDandIS_DBUPDATE_IMAGE_PUSHfollow the same grammar issue (“BUILDED”) asIS_IMAGE_BUILDED. Also, declaring these as top-level Groovy vars may lead to scoping issues acrossscriptblocks. Consider renaming toIS_DBUPDATE_IMAGE_BUILT/IS_IMAGE_BUILT/IS_IMAGE_PUSHED, etc., and moving flag state into theenvironmentblock or leveragingcurrentBuildproperties for persistence.
168-185:⚠️ Potential issueFail build on critical vulnerabilities
The|| trueat the end of the Trivy command masks exit codes so HIGH/CRITICAL findings won’t fail the pipeline. To align with your security policy, remove|| trueor explicitly handle the exit code to fail on vulnerability detection.sh """ docker run --rm \ -v /var/run/docker.sock:/var/run/docker.sock \ - aquasec/trivy image --no-progress --severity HIGH,CRITICAL --exit-code 1 ${image} || true + aquasec/trivy image --no-progress --severity HIGH,CRITICAL --exit-code 1 ${image} """
🧹 Nitpick comments (3)
Jenkinsfile (3)
16-17: Consolidate credential usage for Discord webhook
You bindDISCORD_WEBHOOK_URLat the pipeline level and again insidesendDiscordNotification. To avoid duplication, either remove the outer credential injection and rely on the innerwithCredentials, or drop the inner binding and referenceenv.DISCORD_WEBHOOK_URLdirectly.
256-259: Remove outdated commented code
The commented-outsendDiscordNotificationinvocation is no longer needed and clutters the stage. Deleting these lines will improve readability.
460-482: Parameterize environment and improve error handling in notifications
- The
"Environment": "Stage"field is hard-coded; accept anenvironmentparameter to support multiple targets.- You can simplify credential management by using the pipeline’s
DISCORD_WEBHOOK_URLdirectly, removing the nestedwithCredentials.- Rather than shelling out with
curl, leverage the JenkinshttpRequeststep andgroovy.json.JsonOutputfor safer JSON construction and built-in error handling.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Jenkinsfile(8 hunks)
🔇 Additional comments (7)
Jenkinsfile (7)
144-149: Correct branch regex for multi-digit versions
Great use of\\d+\\.\\d+\\.\\d+to allow multi-digit release branches (e.g.,release/10.12.1). This fully addresses earlier feedback.
153-162: Build both images securely and tag with version
ThewithCredentialsscope for Docker registry credentials andenv.DOCKER_USERNAMEusage are spot-on. Building the backend anddbupdateimages side-by-side with consistent tagging is implemented correctly.
195-206: Push both images only when built successfully
Thewhenguard correctly ensures both images are built before pushing, and pushing thedbupdateimage along with settingIS_DBUPDATE_IMAGE_PUSHis consistent with the build stage.
218-219: Deploy only after both images are pushed
UsingIS_IMAGE_PUSHandIS_DBUPDATE_IMAGE_PUSHin thewhenblock properly gates theDeploy Stage. This alignment between push flags and deployment is correct.
242-242: Includedbupdatein container stop command
Addingdbupdateensures the old database-update container is stopped before bringing up the new stack. This is necessary and correctly applied.
271-273: Consistent gating for the next-step stage
Reusing the samewhenexpression forWHAT IS THE NEXT STEPensures the production choice prompt only appears after both images are pushed. This consistency is well-implemented.
293-301: Avoid input inpostonaborted
Callinginputinside apost { aborted { … } }block may not execute as expected since the pipeline is already in an aborted state. Consider capturing the rollback decision earlier or using a separate rollback stage.
| failure { | ||
| script { | ||
| sendDiscordNotification('FAILED', 'Unexpected failure in "WHAT IS THE NEXT STEP" stage.') | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use post for failure notifications
Similarly, it’s more robust to declare a stage-level post { failure { … } } section for unexpected failures rather than embedding it in a script block.
| if (currentBuild.result == 'SUCCESS') { | ||
| sendDiscordNotification('SUCCESS', 'Deployment to Stage completed successfully.') | ||
| } else if (currentBuild.result == 'ABORTED') { | ||
| sendDiscordNotification('ABORTED', 'Deployment to Stage was aborted.') | ||
| } else if (currentBuild.result == 'FAILURE') { | ||
| sendDiscordNotification('FAILED', 'Deployment to Stage failed.') | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Move status notifications to post
Embedding notifications inside the script block based on currentBuild.result can be unreliable on failure or abort. It’s clearer to define a stage-level post { success { … } failure { … } aborted { … } } block for notifications.
Resolved an issue where the pipeline failed at dotnet tool update due to dotnet-coverage version 17.13.1 already being installed
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
Jenkinsfile (2)
153-160: Branch pattern correctly supports multi-digit versions
The regexrelease/\\d+\\.\\d+\\.\\d+now properly handles version segments like10.12.1. 🎉
190-195:⚠️ Potential issuePrevent masking of critical vulnerabilities in Trivy scan
By appending|| true, the pipeline will never fail on HIGH/CRITICAL findings, potentially overlooking release-blocking issues. To enforce policy, remove the|| trueor explicitly check the exit code:- aquasec/trivy image --no-progress --severity HIGH,CRITICAL --exit-code 1 ${image} || true + aquasec/trivy image --no-progress --severity HIGH,CRITICAL --exit-code 1 ${image}
🧹 Nitpick comments (9)
Jenkinsfile (9)
16-18: Remove redundant credential lookup for Discord webhook
You loadDISCORD_WEBHOOK_URLviacredentials('WEBHOOK_URL')in theenvironmentblock, yetsendDiscordNotificationalso callswithCredentials. You can remove the inner lookup and referenceenv.DISCORD_WEBHOOK_URLto simplify and avoid duplicate credential fetches.
52-60: Consolidatedotnet-coverageinstallation logic
Instead of manually checking ifdotnet-coverageis installed, use theupdatecommand which installs if missing or updates when present:- def coverageInstalled = sh(... | grep dotnet-coverage) == 0 - if (!coverageInstalled) { - sh 'dotnet tool install --global dotnet-coverage --version 17.13.1' - } else { - echo 'dotnet-coverage is already installed.' - } + sh 'dotnet tool update --global dotnet-coverage --version 17.13.1'This reduces branching and ensures idempotency.
162-171: Extract image build steps into a reusable function
The build logic forstreetcodeanddbupdateimages is nearly identical. Consider creating a helper method to DRY this up:def buildImage(name, dockerfile) { sh "docker build -f ${dockerfile} -t ${username}/${name}:${env.CODE_VERSION} ." } script { withCredentials([...]) { IS_IMAGE_BUILT = buildImage('streetcode', 'Dockerfile') IS_DBUPDATE_IMAGE_BUILT = buildImage('dbupdate', 'Dockerfile.dbupdate') } }
258-264: Move notifications to stage-levelpostblocks
Embedding Discord notifications inside thescriptblock depends oncurrentBuild.resultand may not fire reliably. Consider using:stage('Deploy Stage') { ... post { success { sendDiscordNotification('SUCCESS', 'Deployment to Stage completed successfully.') } failure { sendDiscordNotification('FAILED', 'Deployment to Stage failed.') } aborted { sendDiscordNotification('ABORTED','Deployment to Stage was aborted.') } } }This guarantees execution.
279-284: Rename stage for clarity
The titleWHAT IS THE NEXT STEPis non-standard. Rename it to something likeNext ActionsorPromotion Choiceto improve readability and make its purpose evident.
291-309: Streamline rollback logic in aborted post
The rollback steps duplicate cleanup commands from the deploy stage. Extract common logic into a function orpostblock to reduce duplication and improve maintainability.
314-318: Ensure failure notifications are always sent
PlacesendDiscordNotification('FAILED', …)inside apost { failure { … } }block rather than a manualscriptconditional to ensure it triggers on any stage error.
445-460: Remove or re-enable commented global notifications
Thepost { success/failure/aborted }section is commented out, disabling pipeline-wide alerts. Either restore these notifications usingsendDiscordNotificationor delete the dead code to keep the Jenkinsfile clean.
469-491: RefactorsendDiscordNotificationto use JenkinshttpRequest
Usingcurlin a shell step lacks error handling. Replace with the Jenkins HTTP request step andgroovy.json.JsonOutputfor robust payload creation and automatic failure on non-2xx responses:import groovy.json.JsonOutput def sendDiscordNotification(String status, String message) { def payload = JsonOutput.toJson([ content: "$status: $message", embeds: [...] ]) httpRequest( httpMode: 'POST', contentType: 'APPLICATION_JSON', requestBody: payload, url: env.DISCORD_WEBHOOK_URL ) }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Jenkinsfile(9 hunks)
🔇 Additional comments (4)
Jenkinsfile (4)
177-181: Validate both images are built before Scanning
Thewhencondition correctly gates the Trivy scan, preventing null or missing images. Nice catch!
203-205: Push stage gating logic is correct
The push step only runs after both images are built. No changes needed here.
227-231: Expression gating is consistent for multi-image deployment
Thewhencondition correctly checks both push flags before prompting for staging approval.
249-252: Includedbupdatecontainer in cleanup commands
Addingdbupdatetodocker stopensures the new service is redeployed correctly.
| def CODE_VERSION = '' | ||
| def IS_IMAGE_BUILDED = false | ||
| def IS_DBUPDATE_IMAGE_BUILDED = false | ||
| def IS_IMAGE_PUSH = false | ||
| def IS_DBUPDATE_IMAGE_PUSH = false |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor boolean flags and version declarations into environment scope
It's recommended to promote pipeline-wide flags (IS_IMAGE_BUILDED, IS_DBUPDATE_IMAGE_BUILDED, IS_IMAGE_PUSH, IS_DBUPDATE_IMAGE_PUSH, CODE_VERSION) into the environment {} block and rename them for correct grammar (e.g., IS_IMAGE_BUILT). This ensures they persist across stages and reduces scoping issues in script blocks.
| sh 'echo "${password}" | docker login -u "${username}" --password-stdin' | ||
| sh "docker push ${username}/streetcode:latest" | ||
| sh "docker tag ${username}/streetcode:latest ${username}/streetcode:${env.CODE_VERSION}" | ||
|
|
||
| sh "docker push ${username}/streetcode:${env.CODE_VERSION}" | ||
| IS_IMAGE_PUSH = true | ||
|
|
||
|
|
||
| sh "docker push ${username}/dbupdate:${env.CODE_VERSION}" | ||
| IS_DBUPDATE_IMAGE_PUSH = true | ||
|
|
There was a problem hiding this comment.
Fix shell quoting for Docker login and push
Using single quotes prevents Groovy from expanding ${password} and ${username}. Switch to double-quoted Groovy strings so variables interpolate properly:
- sh 'echo "${password}" | docker login -u "${username}" --password-stdin'
+ sh "echo \"${password}\" | docker login -u \"${username}\" --password-stdin"Ensure docker push commands also interpolate correctly.
📝 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.
| sh 'echo "${password}" | docker login -u "${username}" --password-stdin' | |
| sh "docker push ${username}/streetcode:latest" | |
| sh "docker tag ${username}/streetcode:latest ${username}/streetcode:${env.CODE_VERSION}" | |
| sh "docker push ${username}/streetcode:${env.CODE_VERSION}" | |
| IS_IMAGE_PUSH = true | |
| sh "docker push ${username}/dbupdate:${env.CODE_VERSION}" | |
| IS_DBUPDATE_IMAGE_PUSH = true | |
| sh "echo \"${password}\" | docker login -u \"${username}\" --password-stdin" | |
| sh "docker push ${username}/streetcode:${env.CODE_VERSION}" | |
| IS_IMAGE_PUSH = true | |
| sh "docker push ${username}/dbupdate:${env.CODE_VERSION}" | |
| IS_DBUPDATE_IMAGE_PUSH = true |
dev
JIRA
Code reviewers
Second Level Review
Summary of issue
ToDo
Summary of change
ToDo
Testing approach
ToDo
CHECK LIST
Summary by CodeRabbit