Integrated code lifecycle: Insert repository content consistently when preparing for building

[SimonEntholzer]

Checklist

General

• I tested all changes and their related features with all corresponding user types on a test server.
• This is a small issue that I tested locally and was confirmed by another developer on a test server.
• I chose a title conforming to the naming conventions for pull requests.

Server

• Important: I implemented the changes with a very good performance and prevented too many (unnecessary) and too complex database calls.
• I strictly followed the principle of data economy for all database calls.
• I strictly followed the server coding and design guidelines.
• I documented the Java code using JavaDoc style.

Changes affecting Programming Exercises

• High priority: I tested all changes and their related features with all corresponding user types on a test server configured with the integrated lifecycle setup (LocalVC and LocalCI).

Motivation and Context

The current behaviour of repository injection before the build is the following:
During build preparation, the repositories contents are copied to the locations specified by the checkout directories.
Since the mv command is used, the behaviour is different, depending on whether the folder already exists or not.

Example:
Let's assume an exercise, with the following folder and file structure:

We add an auxiliary repository like this:

Now, when the build preparation runs, and the auxiliary repository is copied to the build files, the folder structure looks like this (assuming the auxiliary repository also just contains a "AuxFile.java"):

In case the "filesThatShouldBeReplaced" folder does not exist in the template, the auxiliary repositories files are directly inserted, without the aux repos name, as the directory itself can be renamed (that's how mv works).

This is also the way we want it to work, when the folder does already exist. The auxiliary repository should overwrite existing files.
In our example, this is what we would like to have after the auxiliary repository insertion:

Description

We want a consistent insertion behaviour across all the repositories of an exercise, nonetheless whether the directory specified in the checkout path already exists or not.
To enable this consistent insertion behaviour, before moving the repository content into the build folders, we need to remove the existing files and folder first.

TL:DR now instead of

mv $repository $checkoutDirectory

we do:

rm -rf $checkoutDirectory
mv $repository $checkoutDirectory

We want this to be the standard behaviour, as it is consistent, and does not depend on whether the $checkoutDirectory exists or not.

Steps for Testing

1. Create a programming exercise (java e.g.) with an auxiliary repository, with a checkout path that will replace data from the template. E.g. if the package name is "package", set the auxiliary repository's checkout directory to "assignment/src/package" .
2. Save the exercise. Both the template and solution build plan should fail now.
3. Now, add the files from the solution repository's src/package folder to the auxiliary repository.
4. Both the template and solution build plan should build, and achieve 100%

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

Performance Review

• I (as a reviewer) confirm that the client changes (in particular related to REST calls and UI responsiveness) are implemented with a very good performance even for very large courses with more than 2000 students.
• I (as a reviewer) confirm that the server changes (in particular related to database calls) are implemented with a very good performance even for very large courses with more than 2000 students.

Code Review

• Code Review 1
• Code Review 2

Manual Tests

• Test 1
• Test 2

Exam Mode Test

• Test 1
• Test 2

Test Coverage

Screenshots

Summary by CodeRabbit

Release Notes

• New Features

  • Enhanced directory management in Docker containers for improved build job reliability.
  • Updated styling for tooltips, enhancing user interface clarity.

• Bug Fixes

  • Clarified usage descriptions in German and English localization files regarding auxiliary repositories, emphasizing the overwriting of existing files.

These updates aim to improve user experience and ensure more reliable build processes.

[github-actions]

There hasn't been any activity on this pull request recently. Therefore, this pull request has been automatically marked as stale and will be closed if no further activity occurs within seven days. Thank you for your contributions.

[coderabbitai]

Walkthrough

The pull request introduces significant changes to the BuildJobContainerService class, specifically updating methods for managing directories within Docker containers. The addAndPrepareDirectory method has been replaced with addAndPrepareDirectoryAndReplaceContent, which now ensures existing directories and files are removed before new content is added. Additionally, a new method, removeDirectoryAndFiles, has been added to facilitate this cleanup. Updates to CSS for tooltips and localization files in both German and English have also been made to clarify the functionality of auxiliary repositories in the programming exercises.

Changes

File	Change Summary
src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildJobContainerService.java	- Replaced addAndPrepareDirectory with addAndPrepareDirectoryAndReplaceContent. - Added removeDirectoryAndFiles method for directory cleanup.
src/main/webapp/app/exercises/programming/manage/update/update-components/programming-exercise-information.component.scss	- Added CSS rule for .tooltip-inner to set maximum width to 600 pixels.
src/main/webapp/i18n/de/programmingExercise.json	- Updated usageDescription to clarify that additional code will overwrite existing files.
src/main/webapp/i18n/en/programmingExercise.json	- Enhanced usageDescription to clarify the behavior of auxiliary repositories regarding file overwriting.

Possibly related PRs

• Integrated code lifecycle: Configure checkout path and timeout for programming exercises #9217: This PR modifies the BuildJobContainerService class, which is directly related to the changes made in the main PR that also involves significant modifications to the same class, particularly in methods related to directory preparation and content management.
• Integrated code lifecycle: Improve clean up of temp folders in build agents #9542: This PR enhances the cleanup process of temporary directories in the BuildJobExecutionService, which is relevant to the overall management of build jobs and directories, aligning with the main PR's focus on directory management within Docker containers.

Suggested labels

tests, documentation, programming, config-change, template

Suggested reviewers

• JohannesStoehr
• magaupp
• BBesrour

---

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?

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

• Review comments: Directly reply to a review comment made by CodeRabbit. Example:
  • I pushed a fix in commit <commit_id>, please review it.
  • Generate unit testing code for this file.
  • Open a follow-up GitHub issue for this discussion.
• Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
  • @coderabbitai generate unit testing code for this file.
  • @coderabbitai modularize this function.
• PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
  • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
  • @coderabbitai read src/utils.ts and generate unit testing code.
  • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
  • @coderabbitai help me debug CodeRabbit configuration file.

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)

• @coderabbitai pause to pause the reviews on a PR.
• @coderabbitai resume to resume the paused reviews.
• @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
• @coderabbitai full review to do a full review from scratch and review all the files again.
• @coderabbitai summary to regenerate the summary of the PR.
• @coderabbitai resolve resolve all the CodeRabbit review comments.
• @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
• @coderabbitai help to get help.

Other keywords and placeholders

• Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
• Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
• Add @coderabbitai or @coderabbitai title anywhere in the PR title to generate the title automatically.

Documentation and Community

• Visit our Documentation for detailed information on how to use CodeRabbit.
• Join our Discord Community to get help, request features, and share feedback.
• Follow us on X/Twitter for updates and announcements.

[coderabbitai]

Actionable comments posted: 1

🧹 Outside diff range and nitpick comments (1)

src/main/webapp/i18n/en/programmingExercise.json (1)

538-538: LGTM! Consider adding a note about the overwrite behavior.

The updated description accurately reflects the new standardized behavior of auxiliary repository content insertion. It clearly explains the timing, impact, and mitigation steps.

Consider adding a warning symbol (⚠️) or making "overwrites everything" bold to emphasize the destructive nature of this operation:

-                "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 inserted code overwrites everything which lies at the location specified by the checkout directory. If you only need to overwrite student files partially, you need to adapt the build script.",
+                "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 inserted code **overwrites everything** which lies at the location specified by the checkout directory. If you only need to overwrite student files partially, you need to adapt the build script.",

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 6e1c562 and d6cd825.

📒 Files selected for processing (4)

• src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildJobContainerService.java (2 hunks)
• src/main/webapp/app/exercises/programming/manage/update/update-components/programming-exercise-information.component.scss (1 hunks)
• src/main/webapp/i18n/de/programmingExercise.json (1 hunks)
• src/main/webapp/i18n/en/programmingExercise.json (1 hunks)

🧰 Additional context used

📓 Path-based instructions (2)

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

src/main/webapp/i18n/de/programmingExercise.json (1)

Pattern src/main/webapp/i18n/de/**/*.json: German language translations should be informal (dutzen) and should never be formal (sietzen). So the user should always be addressed with "du/dein" and never with "sie/ihr".

🪛 GitHub Check: Codacy Static Code Analysis

src/main/webapp/app/exercises/programming/manage/update/update-components/programming-exercise-information.component.scss

[warning] 5-5: src/main/webapp/app/exercises/programming/manage/update/update-components/programming-exercise-information.component.scss#L5
Unexpected unknown pseudo-element selector "::ng-deep" (selector-pseudo-element-no-unknown)

🔇 Additional comments (3)

src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildJobContainerService.java (2)

303-313: LGTM: Consistent directory content replacement implementation.

The changes standardize the behavior of repository content insertion by using addAndPrepareDirectoryAndReplaceContent consistently across all repository types (test, assignment, solution, and auxiliary repositories).

---

324-329: Consider potential race conditions in directory operations.

While the implementation is logically correct, concurrent container operations might lead to race conditions between directory removal and rename operations. Consider implementing atomic operations or adding synchronization mechanisms to ensure thread safety.

Run this script to check for concurrent container operations:

src/main/webapp/i18n/de/programmingExercise.json (1)

538-538: LGTM! The translation accurately describes the new behavior.

The German translation:

• Uses the correct informal language style ("du/dein")
• Clearly explains that auxiliary repository content overwrites existing files
• Properly warns users about the need to adjust build scripts for partial overwrites
• Aligns with the PR objective of standardizing repository content insertion behavior

[HawKhiem]

Tested on TS4. At first the template and solution repos were both 0% as described. After adding the mentioned files into the auxilary repo, both the template and solution repos build with a 100% as expected.

[krusche]

Code looks good to me 👍