-
Notifications
You must be signed in to change notification settings - Fork 61
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
chore(core): Make Docker & CI affect everything #17078
Conversation
WalkthroughThe changes made in this pull request focus on modifying the Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #17078 +/- ##
=======================================
Coverage 35.68% 35.68%
=======================================
Files 6941 6940 -1
Lines 147232 147226 -6
Branches 41863 41860 -3
=======================================
+ Hits 52538 52540 +2
+ Misses 94694 94686 -8 Flags with carried forward coverage won't be shown. Click here to find out more. see 3 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Datadog ReportAll test runs ✅ 99 Total Test Services: 0 Failed, 91 Passed Test ServicesThis report shows up to 10 services
🔻 Code Coverage Decreases vs Default Branch (2) |
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.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
nx.json (1)
155-163
: Consider including additional Docker-related filesThe current implementation provides good coverage of CI and Dockerfile changes. However, consider expanding the "Dockerfiles" input to include other Docker-related files.
Consider updating the "Dockerfiles" input:
"Dockerfiles": [ "{workspaceRoot}/**/Dockerfile", - "{workspaceRoot}/**/Dockerfile.*" + "{workspaceRoot}/**/Dockerfile.*", + "{workspaceRoot}/**/docker-compose*.yml", + "{workspaceRoot}/**/.dockerignore" ],
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
nx.json
(2 hunks)
🔇 Additional comments (3)
nx.json (3)
149-149
: LGTM! Default inputs now include DevOps changes
Adding "DevOps" to default inputs ensures consistent tracking of CI/Docker changes across all targets, providing workspace-wide coverage.
98-98
: LGTM! Build target now correctly tracks DevOps changes
The addition of "DevOps" to build inputs ensures that changes to CI/Docker files will trigger rebuilds of affected applications, directly addressing the PR objective.
Let's verify the build dependencies are correctly configured:
✅ Verification successful
Build target configuration is correctly set up for DevOps changes
The verification confirms that:
- The build target in nx.json correctly includes "DevOps" in its inputs
- "DevOps" is properly defined in namedInputs to include both CI and Dockerfile changes
- The configuration is consistently applied across the workspace
The changes will ensure that modifications to CI workflows and Dockerfiles trigger rebuilds of affected applications as intended.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify build dependencies configuration
# Check if the build target is properly configured to track DevOps changes
# Search for any other build configurations that might need similar updates
rg -g '*.json' '"build":\s*{' -A 5
Length of output: 124
Script:
#!/bin/bash
# Let's try a different approach to verify build configurations
# 1. Search for build configurations in JSON files
rg -g '*.json' '"build"' -A 5
# 2. Also verify the complete structure of nx.json to understand the full context
cat nx.json
# 3. Check for any other files that might reference DevOps configurations
rg -g '*.json' 'DevOps'
Length of output: 33827
Line range hint 98-163
: Verify the complete dependency chain
The changes effectively ensure that CI and Docker changes affect applications. Let's verify the complete dependency chain is working as expected.
✅ Verification successful
Configuration verified: DevOps changes properly integrated into build chain
The verification confirms that:
- The build target correctly includes "DevOps" in its inputs, ensuring Docker and CI changes trigger rebuilds
- Dockerfile paths exist in multiple locations, validating the "Dockerfiles" named input
- No conflicting configurations found (nx.json is the only config file with these settings)
- The dependency chain is properly structured without circular dependencies
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the complete dependency chain
# Check if all necessary files are included in the dependency tracking
# Check for any project-specific nx configuration files that might override these settings
fd -e json -x grep -l "targetDefaults\|namedInputs" {}
# Verify no conflicting configurations exist
rg -g '*.json' '"DevOps":\s*\[' -C 5
Length of output: 437
Script:
#!/bin/bash
# Let's verify the complete chain by checking:
# 1. If build target correctly includes DevOps inputs
# 2. If there are no circular dependencies
# 3. If the paths in named inputs actually exist
# Check build target configuration
rg -g 'nx.json' '"build":\s*{' -A 5
# Check if the DevOps paths exist
fd -p '.github/actions/'
fd -p '.github/workflows/'
fd -p 'Dockerfile'
# Verify the inputs are properly referenced
rg -g 'nx.json' '"inputs":\s*\[.*"DevOps"' -C 2
Length of output: 543
Affected services are: air-discount-scheme-api,air-discount-scheme-backend,api,application-system-api,download-service,financial-aid-api,financial-aid-backend,financial-aid-open-api,github-actions-cache,icelandic-names-registry-backend,judicial-system-api,judicial-system-digital-mailbox-api,judicial-system-backend,judicial-system-message-handler,judicial-system-scheduler,judicial-system-xrd-api,license-api,reference-backend,regulations-admin-backend,services-auth-admin-api,services-auth-ids-api,services-auth-delegation-api,services-auth-personal-representative,services-auth-personal-representative-public,services-auth-public-api,services-bff,services-contentful-entry-tagger,services-documents,services-endorsements-api,services-form-system,services-search-indexer,services-sessions,services-university-gateway,services-user-notification,services-user-profile,services-xroad-collector,skilavottord-ws,air-discount-scheme-web,auth-admin-web,consultation-portal,contentful-apps,financial-aid-web-osk,financial-aid-web-veita,judicial-system-web,skilavottord-web,web,application-system-form,island-ui-storybook,portals-admin,service-portal,system-e2e,external-contracts-tests, Deployed services: application-system-api,application-system-form,service-portal,service-portal-api,portals-admin,consultation-portal,api,web,xroad-collector,skilavottord-web,skilavottord-ws,services-documents,island-ui-storybook,download-service,icelandic-names-registry-backend,endorsement-system-api,air-discount-scheme-web,air-discount-scheme-backend,air-discount-scheme-api,regulations-admin-backend,external-contracts-tests,application-system-api-worker,license-api,services-sessions,services-sessions-worker,services-sessions-cleanup,services-university-gateway,services-university-gateway-worker,services-bff-portals-admin. |
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.
scripts/* should also 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.
Approving with comment.
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.
LGTM
Recent
main
outages have been caused by regressions in CI and Dockerfiles. These core components currently don't "affect" the applications, hence, the applications aren't built, tested, docker-built, etc. when they should be.Hoping here to catch future regressions.
Summary by CodeRabbit
New Features
Bug Fixes