update Forge container and sample code#35
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughMoves the impersonation success-path into the impersonation try block so related failures are caught; removes the app.connect bridge and adjusts Forge runtime/routing in the manifest; renames the container health endpoint path from /ping to /health. ChangesExample App API and Configuration Updates
🎯 3 (Moderate) | ⏱️ ~20 minutes
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Up to standards ✅🟢 Issues
|
- Set explicit environment variables (APP_ID, ENV_NAME, ENV_ID) in `dev-loop.sh`. - Simplify `manifest.yml` by removing unused `connect` block and updating routes. - Adjust `/health` endpoint path in `ContainerHealthEndpoint`. - Refactor `BusinessLogicController` for improved exception handling and logic flow.
vzakharchenko
force-pushed
the
container_health_check_fix
branch
from
ed310b4 to
01bd272
Compare
![codacy-production[bot]](https://avatars.githubusercontent.com/in/56611?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull Request Overview
While the PR is technically up to Codacy standards, there are several critical issues that should prevent merging. Most significantly, the 'dev-loop.sh' file and its associated environment variable changes—explicitly mentioned in the acceptance criteria—are missing from the diff. Additionally, the route path update in 'manifest.yml' from '/api/impersonation' to '/api' constitutes a breaking change that likely mismatches existing backend Spring controller mappings, which will lead to 404 errors for the Forge-to-Remote bridge. The refactored error handling in 'BusinessLogicController' also violates standard HTTP practices by returning 200 OK responses for failed API calls, effectively hiding errors from the client.
About this PR
- The PR description and acceptance criteria mention setting explicit environment variables (APP_ID, ENV_NAME, ENV_ID) in a 'dev-loop.sh' file, but this file is missing from the provided changes. Please verify if the file was forgotten in the commit.
- There are no new or updated tests included to verify the behavior of the refactored controller logic or the modified health endpoint. Given the changes to error handling and routing, automated unit or integration tests are recommended.
Test suggestions
- Verify that the /health endpoint returns a 200 OK status
- Verify that failures in fetchJiraMyself are caught and return the specified error message in BusinessLogicController
- Verify that the application starts correctly with the simplified manifest.yml structure
Prompt proposal for missing tests
Consider implementing these tests if applicable:
1. Verify that the /health endpoint returns a 200 OK status
2. Verify that failures in fetchJiraMyself are caught and return the specified error message in BusinessLogicController
3. Verify that the application starts correctly with the simplified manifest.yml structure
TIP Improve review quality by adding custom instructions
TIP How was this review? Give us feedback
|
Coverage Impact This PR will not change total coverage. 🚦 See full report on Qlty Cloud »🛟 Help
|
There was a problem hiding this comment.
Pull Request Overview
The PR streamlines the Forge sample application but introduces several concerns that should prevent merging in its current state. Most notably, 'dev-loop.sh' is missing from the diff despite being a core part of the acceptance criteria, and no tests have been provided despite the checklist indicating otherwise.
There are also two breaking changes: the renaming of the health endpoint and the modification of the base route path in the manifest. These changes will likely break infrastructure monitoring and existing client integrations if not managed. Finally, a hardcoded App ID was found in the manifest, contradicting the project's security checklist.
About this PR
- The file 'dev-loop.sh' is missing from the PR despite being mentioned in the description and acceptance criteria. Please ensure all intended files are included in the commit.
- The PR checklist claims new or updated unit tests are included, but no such files were found in the diff. Please include the relevant tests to verify the changes, specifically for the error handling updates in BusinessLogicController.
Test suggestions
- Verify the health endpoint is reachable at /health and no longer at /ping.
- Verify that exceptions in fetchJiraMyself are caught and returned as 'Impersonation failed' messages.
- Verify that the Forge application correctly routes requests to the backend service with the updated manifest paths.
Prompt proposal for missing tests
Consider implementing these tests if applicable:
1. Verify the health endpoint is reachable at /health and no longer at /ping.
2. Verify that exceptions in fetchJiraMyself are caught and returned as 'Impersonation failed' messages.
3. Verify that the Forge application correctly routes requests to the backend service with the updated manifest paths.
TIP Improve review quality by adding custom instructions
TIP How was this review? Give us feedback
|
1 participant
dev-loop.sh.manifest.ymlby removing unusedconnectblock and updating routes./healthendpoint path inContainerHealthEndpoint.BusinessLogicControllerfor improved exception handling and logic flow.Summary
Fixes #
Type of change
examples/atlassian-connect-forge-spring-boot-sample/)Modules touched
bridge-commonbridge-forge-connect(Connect / Forge Remote)bridge-connect-container(Forge Containers)README,CONTRIBUTING, …)Breaking change
How tested
mvn spotless:apply mvn clean install # If examples/ changed: mvn clean install -f examples/atlassian-connect-forge-spring-boot-sample/pom.xmlmvn spotless:apply+mvn clean installat repo rootexamples/changed)Checklist
mainand has a focused scope (one topic when possible)app.id, production URLs, or personal deploy scripts committedSummary by CodeRabbit
Bug Fixes
Configuration