-
Notifications
You must be signed in to change notification settings - Fork 303
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
Development
: Add Atlas Profile with optionally autowired Atlas Java API
#9936
base: develop
Are you sure you want to change the base?
Conversation
…Exception (using lambdas)
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.
Tested locally, works as expected with Atlas disabled
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.
tested locally, works as described
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.
tested loclly according to the testing steps. Works as expected
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.
I think I found one unintended change.
Otherwise, I just have two questions.
src/main/java/de/tum/cit/aet/artemis/atlas/repository/ScienceEventRepository.java
Show resolved
Hide resolved
src/main/resources/config/liquibase/changelog/20250204112000_changelog.xml
Show resolved
Hide resolved
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.
Thanks for answering my questions. LGTM now.
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.
Code LGTM 👍
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.
The coverage fails. There seems to be one new class that has 0 Coverage and thus it fails the complete verification. Please adjust the threshold.
071855e
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
🧹 Nitpick comments (1)
gradle/jacoco.gradle (1)
2-2
: Consider tracking coverage improvement goals.The TODOs indicate ambitious targets (90% instruction coverage, 0 class coverage), but there's no clear timeline or tracking mechanism. Consider:
- Converting these TODOs into tracked issues
- Creating a gradual improvement plan with milestones
Would you like me to help create a template for tracking these coverage improvement goals?
Also applies to: 4-4
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
gradle/jacoco.gradle
(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: Call Build Workflow / Build and Push Docker Image
- GitHub Check: Call Build Workflow / Build .war artifact
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: client-tests-selected
- GitHub Check: server-style
- GitHub Check: client-tests
- GitHub Check: server-tests
- GitHub Check: Analyse
🔇 Additional comments (2)
gradle/jacoco.gradle (2)
3-3
: LGTM! Gradual improvement in code coverage thresholds.The incremental increases in coverage thresholds for both aggregated (70) and core module (18) demonstrate a commitment to improving code quality through better test coverage.
Also applies to: 11-11
213-261
: LGTM! Well-structured coverage reporting with security considerations.The implementation demonstrates good practices:
- Secure XML parsing without DOCTYPE
- Clear error handling and reporting
- Informative coverage statistics output
PR is ready to merge, but depends on admins adding the atlas profiles to run configs on
Until then, this PR is blocked. |
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.
Tested locally. Works as described
Breaking Change:
atlas
profile needs to be enabled after merging this PR. Otherwise, all atlas-related functionality is disabled (client & server).Checklist
General
Server
Changes affecting Programming Exercises
Motivation and Context
To allow starting Artemis without atlas, we would need to optionally autowire the atlas Java APIs introduced in !9752.
Description
For this, we need to add a new Spring profile specific to atlas (
PROFILE_ATLAS
) and pass in the atlas Java APIs as Optionals to the Spring components constructor. Depending on whether the call is crucially required, anApiNotPresentException
is thrown. This, for instance, is not required when updating the competency progress (asynchronously).I have tested the following reasons for data inconsistencies:
Initials start without atlas
Task created
Lecture with unit created
With profile: Course created
Steps for Testing
These changes need to be tested locally OR on test server 3.
Artemis__Server_.xml
), changes as part of this PR.Testserver States
Note
These badges show the state of the test servers.
Green = Currently available, Red = Currently locked
Click on the badges to get to the test servers.
Review Progress
Code Review
Manual Tests
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Documentation
Refactor
These enhancements aim to provide a more streamlined and feature-rich course management experience for end-users.