Programming exercises: Only replace existing files when populating build plan repositories
#9968
Conversation
github-actions
bot
added
server
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 pmd (7.7.0)src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildJobContainerService.javaThe following rules are missing or misspelled in your ruleset file category/vm/bestpractices.xml: BooleanInstantiation, DontImportJavaLang, DuplicateImports, EmptyFinallyBlock, EmptyIfStmt, EmptyInitializer, EmptyStatementBlock, EmptyStatementNotInLoop, EmptySwitchStatements, EmptySynchronizedBlock, EmptyTryBlock, EmptyWhileStmt, ExcessiveClassLength, ExcessiveMethodLength, ImportFromSamePackage, MissingBreakInSwitch, SimplifyBooleanAssertion. Please check your ruleset configuration. WalkthroughThe Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildJobContainerService.java (1)Pattern 🔇 Additional comments (2)src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildJobContainerService.java (2)
The method correctly uses path components and maintains the single responsibility principle.
Add path validation and copy verification. While the implementation is clean and efficient, consider these security and reliability improvements:
private void insertRepositoryFiles(String containerId, String oldName, String newName) {
+ checkPath(oldName);
+ checkPath(newName);
executeDockerCommand(containerId, null, false, false, true, "cp", "-r", oldName + (oldName.endsWith("/") ? "." : "/."), newName);
+ // Verify copy operation
+ executeDockerCommand(containerId, null, false, false, true, "test", "-d", newName);
}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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildJobContainerService.java (1)
343-343: LGTM! Consider adding error handling.The simplified approach using
insertRepositoryFilesis more efficient than the previous implementation. However, consider adding error handling for cases where the source directory doesn't exist or the copy operation fails.- insertRepositoryFiles(containerId, LOCALCI_WORKING_DIRECTORY + "/" + repositoryPath.getFileName().toString(), newDirectoryName); + String sourcePath = LOCALCI_WORKING_DIRECTORY + "/" + repositoryPath.getFileName().toString(); + try { + insertRepositoryFiles(containerId, sourcePath, newDirectoryName); + } catch (LocalCIException e) { + throw new LocalCIException("Failed to copy repository files from " + sourcePath + " to " + newDirectoryName, e); + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildJobContainerService.java(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildJobContainerService.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
🔇 Additional comments (1)
src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildJobContainerService.java (1)
343-348: Verify behavior in exam mode.
The changes look good and improve efficiency. However, since the PR mentions exam mode testing requirements, please verify that these file operations maintain the expected behavior during exams, particularly when:
- Multiple students submit simultaneously
- Network connectivity is limited
- Container resources are constrained
✅ Verification successful
Based on the search results, I can generate the final response since I have enough information to verify the exam mode behavior:
File operations are safe for exam mode scenarios
The codebase shows that exam mode is well-handled across the application with specific considerations for:
-
Network connectivity issues:
- Local saving and 30-second auto-sync for student work
- Offline work support for text, quiz and modeling exercises
- Repository operations only require connectivity during initial clone and final submission
-
Concurrent submissions:
- Each student gets a unique exam with randomized exercise order
- Container operations are isolated per student (unique containerId)
- File operations use container-specific paths
-
Resource constraints:
- Container operations are minimal - single
cpcommand for file transfer - No redundant operations or resource-intensive processes in the implementation
- Container operations are minimal - single
The changes maintain these safeguards while improving efficiency through the simplified file replacement approach.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for exam mode specific configurations and tests
rg -A 5 "exam.*mode|isExam"
# Check for concurrent submission handling
rg -A 5 "concurrent|simultaneous|parallel"
# Check for resource constraints
ast-grep --pattern 'class $_ implements $_ {
$$$
private $_ buildJobContainerId;
$$$
private void $_(String containerId, $_) {
$$$
resources
$$$
}
}'
Length of output: 69120
src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildJobContainerService.java
Show resolved
Hide resolved
github-actions
bot
added
the
client
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
src/main/webapp/i18n/en/programmingExercise.json (1)
581-581: Improve clarity by removing redundant sentence.The description effectively explains how auxiliary repositories work, but contains a repetitive sentence.
Apply this diff to remove the redundancy:
- "usageDescription": "You can use auxiliary repositories to provide additional code that students cannot see or modify. The additional code is inserted into the specified checkout directory before the submission is built. The additional code is inserted in the specified checkout directory before the build is created. The inserted files overwrite all files in the checkout directory which have the same name. Files that are not overwritten are retained.", + "usageDescription": "You can use auxiliary repositories to provide additional code that students cannot see or modify. The additional code is inserted into the specified checkout directory before the build is created. The inserted files overwrite all files in the checkout directory which have the same name. Files that are not overwritten are retained.",
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
src/main/webapp/i18n/de/programmingExercise.json(1 hunks)src/main/webapp/i18n/en/programmingExercise.json(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/main/webapp/i18n/de/programmingExercise.json
…ve-not-overwritten-files
…ve-not-overwritten-files
HawKhiem
temporarily deployed
to
artemis-test1.artemis.cit.tum.de
GitHub Actions
Inactive
SimonKaran13
temporarily deployed
to
artemis-test1.artemis.cit.tum.de
GitHub Actions
Inactive
There was a problem hiding this comment.
I tested on TS1 and it does not work as expect. I followed the instructions and created an aux repo. And I get a build failed in the solution repo. It says that the BubbleSort class is duplicate. Additionally, it is not possible to use usual java package names that contain '.' like 'de.tum.cit.xyz' because you are not allowed to set as the check out directory path.
Aux Repo checkout path:
Aux repo BubbleSort.java:
Solution repo BubbleSort.java
Solution Repo build output:
The problem is that you added the BubbleSort.java in the auxiliary reposiotory, within its own src/testPackage folders. This means the whole folder is injected, and you end up with two identical files, although in different folder places. This is what you end up with (excluding all other files): By removing the folders from the auxiliary repository, and having the BubbleSort.java at the directory root, it should work as described. |
SimonKaran13
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
GitHub Actions
Inactive
bassner
deleted the
feature/programming-exercises/do-not-remove-not-overwritten-files
branch
Checklist
General
Server
Client
Changes affecting Programming Exercises
Motivation and Context
In #9521 the build plan population of programming was changed, to provide a consistent insertion of repository content during build plan population.
When inserting the contents of an auxiliary repository, at the moment, all content is replaced, even files which do not exist in the auxiliary repository. (First
rmand thenmv)Description
We want to retain files which are not overwritten.
To do this, this PR changes the
rm+mvto a singlecp -roperation. This overwrites files with the same name, and retains files not present in the auxiliary repository.Example with a simple template repository and a simple auxiliary repository with nested directories:
(First screenshot is the directory structure, the second shows the file's content.)
Before copying the auxiliary repository:
After copying the auxiliary repository into the template repository with
cp -r auxrepo/. assignment/src/:Notice, that after this operation, f2 and file2 were overwritten by the operation and have a different content now.
Steps for Testing
Expected build results:
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
Test Coverage
Server
Summary by CodeRabbit
Summary by CodeRabbit