Integrated code lifecycle: Configure checkout path and timeout for programming exercises

[BBesrour]

Contains migration

Checklist

General

• I tested all changes and their related features with all corresponding user types on a test server.
• Language: I followed the guidelines for inclusive, diversity-sensitive, and appreciative language.
• 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) database calls.
• I strictly followed the server coding and design guidelines.
• I added multiple integration tests (Spring) related to the features (with a high test coverage).
• I added pre-authorization annotations according to the guidelines and checked the course groups for all new REST Calls (security).
• I documented the Java code using JavaDoc style.

Client

• Important: I implemented the changes with a very good performance, prevented too many (unnecessary) REST calls and made sure the UI is responsive, even with large data.
• I strictly followed the client coding and design guidelines.
• Following the theming guidelines, I specified colors only in the theming variable files and checked that the changes look consistent in both the light and the dark theme.
• I added multiple integration tests (Jest) related to the features (with a high test coverage), while following the test guidelines.
• I documented the TypeScript code using JSDoc style.
• I added multiple screenshots/screencasts of my UI changes.
• I translated all newly inserted strings into English and German.

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

Second PR of this feature proposal: #8887

We want the instructors to be able to customize the build timeout and the checkout paths

Steps for Testing

Quick testing (On test servers)

Prerequisites:

• 1 Instructor

Checkout Paths testing

1. Go to programming exercise creation
2. Change the checkout paths:
   • Adding nested paths (e.g. assignment_1/assignment_2) may lead to a build failure. This is expected as it is up for the instructor to adjust the test code/script.
   • For testing purposes, just change the name without nesting (for example change 'assignment' to 'exerciseCode')
3. Click on Customize Build Script, If the build script uses the default checkout paths (assignment/solution/tests) make sure to change them accordingly
4. Create exercise and make sure that builds don't fail. However, we expect failures in these programming languages: Haskell, C.
5. Repeat previous steps for different programming languages and project types, please write the ones you tested in your comment.

Timeout testing

1. Edit the timeout, and then run a build for that exercise. The build duration may exceed the timeout as there are pre-processing steps, the cancellation steps take a few seconds.
2. Make sure that other programming exercises (those that don't have a checkout path/timeout set) work normally.

Testing Locally (Optional)

Prerequisites:

• 1 Instructor
• Go to BuildJobContainerService.java, comment out line 104 and uncomment line 105

//                .withCmd("sh", "-c", "while [ ! -f " + workingDirectory + "/stop_container.txt ]; do sleep 0.5; done")
                 .withCmd("tail", "-f", "/dev/null") // Activate for debugging purposes instead of the above command to get a running container that you can peek into using

• Go to BuildAgentConfiguration.java, change line 92 to .withAutoRemove(false);

1. Create programming exercise
2. Change checkout paths.
3. In the container's filesystem, check that the exercise was cloned in the right place.

• This can be checked in docker desktop (click on the container, then click on files, you navigate the file system there)
• If you don't have docker desktop, run docker exec -it <mycontainer> sh Navigate using the cli

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
• I (as a reviewer) confirm that the server changes (in particular related to database calls) are implemented with a very good performance

Code Review

• Code Review 1
• Code Review 2

Manual Tests

• Test 1
• Test 2

Screenshots

Summary by CodeRabbit

Summary by CodeRabbit

• New Features

  • Enhanced the build job container setup with customizable checkout paths for assignment, test, and solution repositories.
  • Improved script handling with dynamic placeholder replacements based on the CI environment.
  • Enhanced directory path handling for programming exercises, accommodating various configurations.
  • Introduced stricter validation for editing programming exercises, including timeout and checkout path checks.
  • Added localization support for Docker configurations and build parameters in English and German.

• Bug Fixes

  • Added tests to validate the functionality of the new repository checkout path features.

• Documentation

  • Updated documentation to reflect changes in repository management and configuration options.

• Refactor

  • Improved code readability and modularity through the introduction of utility methods for path manipulation.

[coderabbitai]

Actionable comments posted: 5

Outside diff range comments (1)

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

Line range hint 291-309: Ensure consistent null and blank checks for checkout paths.

The conditions for setting assignmentCheckoutPath, testCheckoutPath, and solutionCheckoutPath differ slightly. For consistency and to avoid potential null pointer exceptions, ensure all conditions uniformly check for isBlank():

// Assignment checkout path
assignmentCheckoutPath = (!StringUtils.isBlank(assignmentCheckoutPath)) ? assignmentCheckoutPath : RepositoryCheckoutPath.ASSIGNMENT.forProgrammingLanguage(programmingLanguage);

// Test checkout path
- String defaultTestCheckoutPath = RepositoryCheckoutPath.TEST.forProgrammingLanguage(programmingLanguage);
- testCheckoutPath = (!StringUtils.isBlank(defaultTestCheckoutPath) && !StringUtils.isBlank(testCheckoutPath)) ? testCheckoutPath : defaultTestCheckoutPath;
+ testCheckoutPath = (!StringUtils.isBlank(testCheckoutPath)) ? testCheckoutPath : RepositoryCheckoutPath.TEST.forProgrammingLanguage(programmingLanguage);

// Solution checkout path
solutionCheckoutPath = (!StringUtils.isBlank(solutionCheckoutPath)) ? solutionCheckoutPath : RepositoryCheckoutPath.SOLUTION.forProgrammingLanguage(programmingLanguage);

This ensures that all checkout paths are set using the same logic, enhancing code readability and reducing confusion.

Review details

Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE

Commits

Files that changed from the base of the PR and between 51bfcc8 and 6e0b361.

Files ignored due to path filters (1)

• src/main/resources/templates/javascript/test/package-lock.json is excluded by !**/package-lock.json

Files selected for processing (3)

• src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildJobContainerService.java (6 hunks)
• src/main/resources/templates/javascript/test/.gitignore (1 hunks)
• src/main/resources/templates/javascript/test/package.json (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 not posted (2)

src/main/resources/templates/javascript/test/.gitignore (1)

3-3: LGTM!

The change from a hardcoded directory name to a dynamic reference using ${studentParentWorkingDirectoryName} aligns with the PR objective of allowing customizable checkout paths for programming exercises. It enhances the flexibility of the build job setup without introducing any apparent issues.

src/main/resources/templates/javascript/test/package.json (1)

9-9: Flexible workspace configuration introduced.

The change from a static "assignment" directory to a dynamic reference ${studentParentWorkingDirectoryName} for the "workspaces" property introduces flexibility in configuring the workspace directory. This can be beneficial for accommodating different project structures or configurations.

Please ensure that the variable ${studentParentWorkingDirectoryName} is properly defined and resolved to a valid directory name to avoid potential issues. Additionally, verify compatibility with existing projects or configurations that may rely on the static "assignment" directory.

[coderabbitai]

Actionable comments posted: 3

Review details

Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE

Commits

Files that changed from the base of the PR and between 6e0b361 and 81eb283.

Files selected for processing (3)

• src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildJobContainerService.java (5 hunks)
• src/main/java/de/tum/cit/aet/artemis/programming/service/BuildScriptProviderService.java (6 hunks)
• src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingExerciseRepositoryService.java (2 hunks)

Additional context used

Path-based instructions (3)

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/java/de/tum/cit/aet/artemis/programming/service/BuildScriptProviderService.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/java/de/tum/cit/aet/artemis/programming/service/ProgrammingExerciseRepositoryService.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 not posted (8)

src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingExerciseRepositoryService.java (2)

667-691: Enhancements to placeholder handling look good!

The changes in this method provide more flexibility in configuring the working directories for different repository types. Key improvements include:

• Using the build config to override default paths
• Special handling for Python to replace "/" with "." in the student working directory
• Additional placeholder for Java with Gradle and Rust to support their directory structures

Overall, these enhancements align with the summary and improve the placeholder replacement logic.

---

Line range hint 692-789: Access control logic based on exercise configuration changes is implemented correctly.

This method plays a crucial role in maintaining the integrity of the programming exercise by ensuring that the repository and participation access rights are updated based on the changes in the exercise configuration.

It correctly handles various scenarios such as:

• Locking access when the start date or due date becomes stricter or offline IDE usage is disallowed.
• Unlocking access when the start date or due date becomes more lenient or offline IDE usage is allowed.

The implementation follows a clear and comprehensive logic to cover all possible combinations of configuration changes.

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

27-27: Updated import to Apache Commons Lang 3 StringUtils

The import statement has been correctly updated to use org.apache.commons.lang3.StringUtils, ensuring compatibility with Apache Commons Lang 3.

---

279-284: Added JavaDoc for new parameters

The JavaDoc now includes the new parameters assignmentCheckoutPath, testCheckoutPath, and solutionCheckoutPath, with clear descriptions of their purpose.

---

287-289: Method signature updated with new parameters

The populateBuildJobContainer method now accepts assignmentCheckoutPath, testCheckoutPath, and solutionCheckoutPath parameters, enhancing configurability.

---

291-292: Correct usage of StringUtils.isBlank for assignmentCheckoutPath

The logic correctly assigns the default path if assignmentCheckoutPath is blank, ensuring that the default path is used when no custom path is provided.

---

307-308: Consistent handling of solutionCheckoutPath

The assignment of solutionCheckoutPath is consistent with assignmentCheckoutPath, correctly using the default path when solutionCheckoutPath is blank.

---

446-449: Refactored getParentFolderPath using Path API

The getParentFolderPath method now utilizes Paths.get(), normalize(), and getParent() for OS-independent path handling and improved readability.

[coolchock]

re-approve after JS template was added

[JohannesStoehr]

Reapprove code

[github-actions]

⚠️ Unable to deploy to test servers ⚠️

Testserver "artemis-test3.artemis.cit.tum.de" is already in use by PR #9343.

[SimonEntholzer]

Works on Ts2, Ocaml works now by adjusting the build script, same for Assembler 👍

[edkaya]

Tested on ts2, works as expected

[coderabbitai]

Actionable comments posted: 5

Review details

Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE

Commits

Files that changed from the base of the PR and between 81eb283 and f54e964.

Files selected for processing (4)

• src/main/webapp/app/exercises/programming/manage/update/programming-exercise-update.component.ts (4 hunks)
• src/main/webapp/app/exercises/programming/manage/update/update-components/programming-exercise-language.component.ts (2 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 (3)

src/main/webapp/app/exercises/programming/manage/update/programming-exercise-update.component.ts (1)

Pattern src/main/webapp/**/*.ts: angular_style:https://angular.io/guide/styleguide;methods_in_html:false;lazy_loading:true;code_reuse:true;tests:meaningful;types:PascalCase;enums:PascalCase;funcs:camelCase;props:camelCase;no_priv_prefix:true;strings:single_quotes;localize:true;btns:functionality;links:navigation;icons_text:newline;labels:associate;code_style:arrow_funcs,curly_braces,open_braces_same_line,indent_4;memory_leak_prevention:true;routes:naming_schema;chart_framework:ngx-charts;responsive_layout:true

src/main/webapp/app/exercises/programming/manage/update/update-components/programming-exercise-language.component.ts (1)

Pattern src/main/webapp/**/*.ts: angular_style:https://angular.io/guide/styleguide;methods_in_html:false;lazy_loading:true;code_reuse:true;tests:meaningful;types:PascalCase;enums:PascalCase;funcs:camelCase;props:camelCase;no_priv_prefix:true;strings:single_quotes;localize:true;btns:functionality;links:navigation;icons_text:newline;labels:associate;code_style:arrow_funcs,curly_braces,open_braces_same_line,indent_4;memory_leak_prevention:true;routes:naming_schema;chart_framework:ngx-charts;responsive_layout:true

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".

Additional comments not posted (11)

src/main/webapp/app/exercises/programming/manage/update/update-components/programming-exercise-language.component.ts (5)

55-56: LGTM: Improved Docker image field retrieval.

The use of the nullish coalescing operator (??) to retrieve the dockerImageField from either component is a good practice. It ensures robust handling of potentially undefined values while maintaining clean code.

---

61-66: LGTM: Enhanced timeout field handling.

The addition of timeoutField retrieval and subscription aligns well with the PR objectives. The code structure is consistent with the existing dockerImageField handling, which is good for maintainability.

Note on setTimeout usage:
The use of setTimeout in the subscription callback is consistent with the existing pattern in the file. While this approach works, consider exploring Angular-specific alternatives like ChangeDetectorRef.detectChanges() or NgZone.run() for potentially better integration with Angular's change detection mechanism.

---

98-101: LGTM: Improved validation for PROFILE_LOCALCI.

The enhanced validation logic now correctly checks both dockerImageField and timeoutField validity. The use of optional chaining and nullish coalescing operators ensures robust handling of potentially undefined values. This change aligns well with the PR objectives to improve the configurability of programming exercise build processes.

---

105-108: LGTM: Consistent validation improvement for PROFILE_AEOLUS.

The validation logic for PROFILE_AEOLUS has been updated consistently with the changes made for PROFILE_LOCALCI. This consistency in approach across different profiles is commendable and contributes to the overall maintainability of the code. The enhanced validation aligns well with the PR objectives, ensuring both Docker image and timeout configurations are properly validated.

---

Line range hint 1-114: Overall assessment: Well-implemented enhancements to programming exercise configuration.

The changes in this file consistently improve the validation logic for Docker image and timeout configurations across different profiles (PROFILE_LOCALCI and PROFILE_AEOLUS). The code demonstrates good practices such as:

1. Consistent use of optional chaining and nullish coalescing operators for robust error handling.
2. Adherence to coding guidelines, including naming conventions and code style.
3. Logical enhancements that align well with the PR objectives.

The implementation maintains a good balance between functionality enhancement and code maintainability. The consistent approach across different profiles is particularly commendable.

Minor suggestion for future consideration:
While the current implementation works well, consider exploring Angular-specific alternatives to setTimeout in reactive contexts, such as ChangeDetectorRef.detectChanges() or NgZone.run(), for potentially better integration with Angular's change detection mechanism.

Overall, these changes significantly contribute to the improved configurability of programming exercise build processes as intended by the PR objectives.

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

143-147: LGTM! Clear and concise timeout information.

The "timeout" section provides clear and concise information about customizing the build timeout. The note about administrative limits potentially overriding the user-set value is an important detail for users to be aware of.

---

131-147: Excellent additions enhancing exercise customization options.

The new "checkoutPath" and "timeout" sections are well-integrated into the existing structure of the file. They provide clear, concise, and valuable information to users about customizing repository checkout paths and build timeouts. The warnings and alerts help users understand the implications of their choices, which is crucial for preventing potential issues.

These additions significantly enhance the flexibility and user control over programming exercises while maintaining consistency with the rest of the localization file.

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

118-129: Gut gemachte Übersetzungen für "checkoutPath"!

Die Übersetzungen sind korrekt und verwenden durchgehend die informelle Anrede ("du"-Form), was den Vorgaben entspricht. Die Texte sind klar und informativ, und die JSON-Struktur ist einwandfrei.

src/main/webapp/app/exercises/programming/manage/update/programming-exercise-update.component.ts (3)

414-414: Properly determine edit mode with isEdit flag

The addition of this.isEdit = segments.some((segment) => segment.path === 'edit'); correctly identifies whether the component is in edit mode based on the route parameters.

---

879-880: Include timeout validation in invalid reasons check

Adding this.validateTimeout(validationErrorReasons); ensures that the timeout value is properly validated and any issues are reported to the user.

---

1115-1119: Function areValuesUnique correctly checks for uniqueness

The function effectively filters out undefined and empty strings and checks for duplicates among checkout paths.

[coderabbitai]

Actionable comments posted: 8

Review details

Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE

Commits

Files that changed from the base of the PR and between 81eb283 and f54e964.

Files selected for processing (4)

• src/main/webapp/app/exercises/programming/manage/update/programming-exercise-update.component.ts (4 hunks)
• src/main/webapp/app/exercises/programming/manage/update/update-components/programming-exercise-language.component.ts (2 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 (3)

src/main/webapp/app/exercises/programming/manage/update/programming-exercise-update.component.ts (1)

Pattern src/main/webapp/**/*.ts: angular_style:https://angular.io/guide/styleguide;methods_in_html:false;lazy_loading:true;code_reuse:true;tests:meaningful;types:PascalCase;enums:PascalCase;funcs:camelCase;props:camelCase;no_priv_prefix:true;strings:single_quotes;localize:true;btns:functionality;links:navigation;icons_text:newline;labels:associate;code_style:arrow_funcs,curly_braces,open_braces_same_line,indent_4;memory_leak_prevention:true;routes:naming_schema;chart_framework:ngx-charts;responsive_layout:true

src/main/webapp/app/exercises/programming/manage/update/update-components/programming-exercise-language.component.ts (1)

Pattern src/main/webapp/**/*.ts: angular_style:https://angular.io/guide/styleguide;methods_in_html:false;lazy_loading:true;code_reuse:true;tests:meaningful;types:PascalCase;enums:PascalCase;funcs:camelCase;props:camelCase;no_priv_prefix:true;strings:single_quotes;localize:true;btns:functionality;links:navigation;icons_text:newline;labels:associate;code_style:arrow_funcs,curly_braces,open_braces_same_line,indent_4;memory_leak_prevention:true;routes:naming_schema;chart_framework:ngx-charts;responsive_layout:true

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".

Additional comments not posted (3)

src/main/webapp/app/exercises/programming/manage/update/update-components/programming-exercise-language.component.ts (1)

55-56: LGTM: Proper handling of potential undefined values.

The use of the nullish coalescing operator (??) to retrieve the dockerImageField from either component is a good practice. It ensures that the code gracefully handles cases where one of the components might be undefined.

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

Line range hint 1-147: Overall changes look good and enhance user experience.

The additions to the localization file for "checkoutPath" and "timeout" are well-structured, informative, and consistent with the existing content. These new entries will help users understand and utilize the new features effectively.

src/main/webapp/app/exercises/programming/manage/update/programming-exercise-update.component.ts (1)

414-414: LGTM

The addition of isEdit correctly sets the edit mode based on the route parameters.