-
Notifications
You must be signed in to change notification settings - Fork 305
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
: Automatically update build queues in local continuous integration
#7836
Development
: Automatically update build queues in local continuous integration
#7836
Conversation
WalkthroughThe recent update reflects an enhancement in the local continuous integration (CI) build queue system within a Java-based project. A new WebSocket service was integrated for real-time communication, and listeners were added to handle item and entry events in the build queue. The web interface was also updated to remove redundant elements, and frontend components now include WebSocket subscription logic for dynamic updates. Changes
Poem
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? TipsChat with CodeRabbit Bot (
|
…queue-representation # Conflicts: # src/main/java/de/tum/in/www1/artemis/service/connectors/localci/LocalCISharedBuildJobQueueService.java
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.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to filter (2)
- src/main/webapp/i18n/de/buildQueue.json
- src/main/webapp/i18n/en/buildQueue.json
Files selected for processing (4)
- src/main/java/de/tum/in/www1/artemis/service/connectors/localci/LocalCISharedBuildJobQueueService.java (5 hunks)
- src/main/java/de/tum/in/www1/artemis/web/websocket/localci/LocalCIBuildQueueWebsocketService.java (1 hunks)
- src/main/webapp/app/localci/build-queue/build-queue.component.html (1 hunks)
- src/main/webapp/app/localci/build-queue/build-queue.component.ts (1 hunks)
Files skipped from review due to trivial changes (1)
- src/main/webapp/app/localci/build-queue/build-queue.component.html
Additional comments: 6
src/main/java/de/tum/in/www1/artemis/web/websocket/localci/LocalCIBuildQueueWebsocketService.java (1)
- 14-55: The class
LocalCIBuildQueueWebsocketService
is well-structured with clear responsibilities for sending WebSocket messages related to build queues. It adheres to Spring's standard practices with appropriate use of@Service
and@Profile
annotations, and dependency injection forWebsocketMessagingService
.src/main/webapp/app/localci/build-queue/build-queue.component.ts (1)
- 12-74: The updates to
BuildQueueComponent
include proper handling of WebSocket subscriptions and unsubscriptions, following Angular's best practices for lifecycle hooks. ThecourseChannels
array is a good approach to track and manage multiple WebSocket channels.src/main/java/de/tum/in/www1/artemis/service/connectors/localci/LocalCISharedBuildJobQueueService.java (4)
- 77-89: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [80-101]
The addition of
LocalCIBuildQueueWebsocketService
and the implementation of Hazelcast listeners inLocalCISharedBuildJobQueueService
are correctly done. The listeners are set up to respond to changes in the build job queue and processing jobs, and they use the WebSocket service to communicate these changes to clients.
409-426: The
QueuedBuildJobItemListener
class correctly handles the addition and removal of build jobs from the queue. It updates the WebSocket clients with the current state of the queue, ensuring real-time data synchronization.429-445: The
ProcessingBuildJobItemListener
class ensures that clients are updated with the current running build jobs. It responds to the addition and removal of jobs in the processing map, maintaining the real-time status of the build process.448-459: The
BuildAgentListener
class updates clients with the current build agent information upon changes. This keeps the clients informed about the available build agents and their status.
|
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 on ts6, 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.
code looks good 👍
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
…queue-representation
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 on ts2. 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.
manually tested on legacy ts11, only with students and instructors, worked as expected
…queue-representation
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.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- src/main/java/de/tum/in/www1/artemis/service/connectors/localci/LocalCISharedBuildJobQueueService.java (5 hunks)
Additional comments: 7
src/main/java/de/tum/in/www1/artemis/service/connectors/localci/LocalCISharedBuildJobQueueService.java (7)
22-23: The addition of
EntryAddedListener
andEntryRemovedListener
imports aligns with the implementation of new listener classes for WebSocket functionality.36-36: The import of
LocalCIBuildQueueWebsocketService
is necessary for the WebSocket communication introduced in this PR.80-86: The constructor has been correctly updated to include
LocalCIBuildQueueWebsocketService
for WebSocket communication. Ensure that all instances ofLocalCISharedBuildJobQueueService
are updated to pass this new dependency.Verification successful
The search for instances where
LocalCISharedBuildJobQueueService
is instantiated did not yield any results. This could mean that the service is not directly instantiated using thenew
keyword, which is common for Spring-managed beans. Instead, it is likely that the service is injected by Spring's dependency injection mechanism.Given this, it's not possible to verify through the provided script whether all instances of
LocalCISharedBuildJobQueueService
have been updated to include the new constructor parameter. A different approach is needed to ensure that the Spring context is correctly configured to inject the new dependency whereLocalCISharedBuildJobQueueService
is used.To verify that the Spring context is correctly configured to handle the updated constructor, we should look for the Spring configuration files or annotations that define the beans.
The output from the script indicates that
LocalCISharedBuildJobQueueService
is annotated with@Service
and@Profile("localci")
, which are Spring annotations used to define a bean within the Spring application context. The@Service
annotation marks the class as a service provider, and the@Profile
annotation suggests that this service is only active when the "localci" profile is used.Since the service is managed by Spring, we do not expect direct instantiations with the
new
keyword. Instead, Spring will automatically handle the injection of dependencies into other beans that requireLocalCISharedBuildJobQueueService
.Given this information, it is not necessary to look for updates to the constructor calls in the codebase, as Spring's dependency injection container will take care of providing the required
LocalCIBuildQueueWebsocketService
bean toLocalCISharedBuildJobQueueService
when it is instantiated.Therefore, the review comment is addressed correctly under the assumption that the Spring context is properly configured to autowire the new dependency. However, without checking the configuration of
LocalCIBuildQueueWebsocketService
bean and whereLocalCISharedBuildJobQueueService
is injected, we cannot fully verify that the new dependency is handled correctly everywhere it is needed.To ensure that
LocalCIBuildQueueWebsocketService
is correctly configured as a Spring bean and injected whereverLocalCISharedBuildJobQueueService
is used, we would need to check the configuration ofLocalCIBuildQueueWebsocketService
and the injection points ofLocalCISharedBuildJobQueueService
.
The output confirms that
LocalCIBuildQueueWebsocketService
is also annotated with@Service
and@Profile("localci")
, indicating that it is a Spring-managed bean that will be instantiated and injected by Spring's dependency injection mechanism when the "localci" profile is active.Additionally, the output shows the
LocalCISharedBuildJobQueueService
constructor with the@Autowired
annotation, which is the point where Spring injects the required dependencies. Since the constructor includesLocalCIBuildQueueWebsocketService
as a parameter, this confirms that Spring is expected to provide an instance ofLocalCIBuildQueueWebsocketService
when creatingLocalCISharedBuildJobQueueService
.There is no additional output showing other injection points for
LocalCISharedBuildJobQueueService
, which suggests that the constructor is the primary (or only) point where dependencies are injected. This is typical for Spring services, where the constructor injection is used to provide all necessary dependencies.Based on the information provided by the script output, it appears that the Spring configuration is set up correctly to handle the new dependency on
LocalCIBuildQueueWebsocketService
withinLocalCISharedBuildJobQueueService
. Therefore, the review comment regarding the constructor update and the need to ensure that all instances are updated to pass the new dependency is verified as correct within the context of Spring's dependency injection.* 98-100: Listeners for queued and processing build jobs, as well as build agents, have been registered correctly to enable WebSocket updates.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that all instances of LocalCISharedBuildJobQueueService are updated with the new constructor parameter. rg "new LocalCISharedBuildJobQueueService\("Length of output: 46
Script:
#!/bin/bash # Search for Spring configuration files or annotations related to LocalCISharedBuildJobQueueService rg "@Bean|@Component|@Service|@Repository" -A 3 | rg "LocalCISharedBuildJobQueueService"Length of output: 579
Script:
#!/bin/bash # Check the configuration of LocalCIBuildQueueWebsocketService bean rg "@Bean|@Component|@Service|@Repository" -A 3 | rg "LocalCIBuildQueueWebsocketService" # Check for injection points of LocalCISharedBuildJobQueueService rg "@Autowired" -A 3 | rg "LocalCISharedBuildJobQueueService"Length of output: 1439
409-427: The
QueuedBuildJobItemListener
class correctly handles WebSocket messages for build job queue events. It sends updates when items are added or removed from the queue.429-445: The
ProcessingBuildJobItemListener
class correctly handles WebSocket messages for processing job events. It sends updates when items are added or removed from the processing jobs map.448-459: The
BuildAgentListener
class correctly handles WebSocket messages for build agent events. It sends updates when build agents are added or removed.
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
Enhancement
: Enable websockets for build job queue representation in local continuous integrationDevelopment
: Automatically update build queues in local continuous integration
Checklist
General
Server
Client
Motivation and Context
Currently, we use REST Calls to update the build job queue table. For this, the user needs to manually refresh the content.
Description
By using WebSockets, the table now updates on its own when new build jobs get queued or start running. For this, a new
LocalCIBuildQueueWebsocketService
was created that sends the BuildQueue over different channels. TheBuildQueueComponent
had to be changed accordingly to subscribe to these channels and receive the content.Steps for Testing
Prerequisites:
Either do this with other reviewers or with multiple tabs/browsers open
As Admin:
Navigate to Server Administration > Build Queue
As Instructor:
For all courses: Navigate to Course Management > Course xy > Build Queue
As Students of those courses:
Submit some Programming Exercises multiple times from all courses.
As an Instructor, you should now automatically see some Build Jobs appearing in the Build Queue. These should, however, only be for this specific course.
As an Admin, you should see all possible Build Jobs from all courses appearing automatically.
Testserver States
Note
These badges show the state of the test servers.
Green = Currently available, Red = Currently locked
Review Progress
Performance Review
Code Review
Manual Tests
Test Coverage
Screenshots
Server Administration > Build Queue:

Navigate to Course Management > Course xy > Build Queue:

Summary by CodeRabbit
Summary by CodeRabbit
New Features
Enhancements
Refactor
Documentation