Integrated code lifecycle: Allow admins to set build timeout options via application properties
#9603
Conversation
github-actions
bot
added
server
WalkthroughThe pull request introduces several changes primarily focused on timeout configurations for build jobs within the application. Key modifications include updating the default timeout value in the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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: 5
🧹 Outside diff range and nitpick comments (8)
src/main/webapp/app/exercises/programming/manage/update/update-components/custom-build-plans/programming-exercise-build-configuration/programming-exercise-build-configuration.component.ts (2)
Line range hint
12-24: Consider enhancing form validation for timeout field.While the input/output declarations look good, consider adding form validation to ensure the timeout value stays within the min/max bounds.
timeoutField = viewChild<NgModel>('timeoutField', { validators: [ Validators.min(this.timeoutMinValue ?? 10), Validators.max(this.timeoutMaxValue ?? 240) ] });
25-28: Consider making timeout configuration properties readonly.Since these properties are only set during initialization, they should be marked as readonly to prevent accidental modifications.
- timeoutMinValue?: number; - timeoutMaxValue?: number; - timeoutDefaultValue?: number; + private readonly timeoutMinValue?: number; + private readonly timeoutMaxValue?: number; + private readonly timeoutDefaultValue?: number;src/main/webapp/app/shared/layouts/profiles/profile-info.model.ts (1)
40-42: Add JSDoc documentation for the new timeout properties.Consider adding JSDoc comments to document the purpose and constraints of these properties, making it easier for other developers to understand their usage.
+ /** Minimum allowed build timeout in seconds (e.g., 10) */ public buildTimeoutMin?: number; + /** Maximum allowed build timeout in seconds (e.g., 360) */ public buildTimeoutMax?: number; + /** Default build timeout in seconds if not specified (e.g., 120) */ public buildTimeoutDefault?: number;src/main/webapp/app/shared/layouts/profiles/profile.service.ts (1)
91-93: Consider grouping related properties together.The build timeout properties are conceptually related but are added at the end of the mapping block. Consider grouping them with other build-related properties for better code organization.
Move these properties near the
buildPlanURLTemplateassignment for better code organization:profileInfo.buildPlanURLTemplate = data.buildPlanURLTemplate; + // Build timeout configuration + profileInfo.buildTimeoutMin = data.buildTimeoutMin; + profileInfo.buildTimeoutMax = data.buildTimeoutMax; + profileInfo.buildTimeoutDefault = data.buildTimeoutDefault; profileInfo.commitHashURLTemplate = data.commitHashURLTemplate;src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildJobManagementService.java (2)
Line range hint
155-159: Enhance timeout validation logic.While the maximum timeout check is implemented, the logic is missing validation for the minimum timeout value (10 seconds) mentioned in the PR objectives. Also, the condition could be more readable if split.
Consider applying this change for better readability and complete validation:
- if (buildJobItem.buildConfig().timeoutSeconds() > 0 && buildJobItem.buildConfig().timeoutSeconds() < this.timeoutSeconds) { + int requestedTimeout = buildJobItem.buildConfig().timeoutSeconds(); + if (requestedTimeout < 10) { // Minimum timeout + buildJobTimeoutSeconds = 10; + } else if (requestedTimeout > this.timeoutSeconds) { // Maximum timeout + buildJobTimeoutSeconds = this.timeoutSeconds; + } else { buildJobTimeoutSeconds = buildJobItem.buildConfig().timeoutSeconds(); - } - else { - buildJobTimeoutSeconds = this.timeoutSeconds; }
Line range hint
1-284: Consider extracting timeout configuration to a dedicated class.The
BuildJobManagementServicehandles multiple responsibilities. To improve maintainability and follow the Single Responsibility Principle, consider extracting the timeout configuration into a dedicated@ConfigurationPropertiesclass.Example implementation:
@ConfigurationProperties(prefix = "artemis.continuous-integration.build-timeout-seconds") @Validated public record BuildTimeoutConfig( @Min(10) int min, @Max(360) int max, @Min(10) @Max(360) int defaultValue ) { public BuildTimeoutConfig { if (min > max) { throw new IllegalArgumentException("min timeout cannot be greater than max timeout"); } if (defaultValue < min || defaultValue > max) { throw new IllegalArgumentException("default timeout must be between min and max"); } } }This approach would:
- Centralize timeout configuration
- Add validation at startup
- Make the configuration more maintainable
- Reduce the responsibilities of BuildJobManagementService
src/main/java/de/tum/cit/aet/artemis/core/config/Constants.java (2)
275-280: Add JavaDoc documentation for the new timeout constants.While the implementation is correct, adding JavaDoc documentation would improve maintainability by explaining:
- The purpose of each constant
- The relationship with application properties
- Any constraints or considerations for their usage
Add documentation like this:
+ /** + * Property key for the minimum build timeout value in seconds that can be configured by instructors. + * Maps to artemis.continuous-integration.build-timeout-seconds.min in application properties. + */ public static final String INSTRUCTOR_BUILD_TIMEOUT_MIN_OPTION = "buildTimeoutMin"; + /** + * Property key for the maximum build timeout value in seconds that can be configured by instructors. + * Maps to artemis.continuous-integration.build-timeout-seconds.max in application properties. + */ public static final String INSTRUCTOR_BUILD_TIMEOUT_MAX_OPTION = "buildTimeoutMax"; + /** + * Property key for the default build timeout value in seconds used when no specific timeout is configured. + * Maps to artemis.continuous-integration.build-timeout-seconds.default in application properties. + */ public static final String INSTRUCTOR_BUILD_TIMEOUT_DEFAULT_OPTION = "buildTimeoutDefault";
275-280: Consider grouping CI-related constants.For better code organization, consider adding a separator comment to group these constants with other CI-related constants like
INFO_BUILD_PLAN_URL_DETAIL.Add a section comment like this:
+ // Constants for CI build configuration public static final String INSTRUCTOR_BUILD_TIMEOUT_MIN_OPTION = "buildTimeoutMin"; public static final String INSTRUCTOR_BUILD_TIMEOUT_MAX_OPTION = "buildTimeoutMax"; public static final String INSTRUCTOR_BUILD_TIMEOUT_DEFAULT_OPTION = "buildTimeoutDefault";
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (2)
src/main/resources/config/application-buildagent.ymlis excluded by!**/*.ymlsrc/main/resources/config/application-localci.ymlis excluded by!**/*.yml
📒 Files selected for processing (7)
- src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildJobManagementService.java (2 hunks)
- src/main/java/de/tum/cit/aet/artemis/core/config/Constants.java (1 hunks)
- src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIInfoContributor.java (2 hunks)
- src/main/webapp/app/exercises/programming/manage/update/update-components/custom-build-plans/programming-exercise-build-configuration/programming-exercise-build-configuration.component.html (1 hunks)
- src/main/webapp/app/exercises/programming/manage/update/update-components/custom-build-plans/programming-exercise-build-configuration/programming-exercise-build-configuration.component.ts (2 hunks)
- src/main/webapp/app/shared/layouts/profiles/profile-info.model.ts (1 hunks)
- src/main/webapp/app/shared/layouts/profiles/profile.service.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildJobManagementService.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_importssrc/main/java/de/tum/cit/aet/artemis/core/config/Constants.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_importssrc/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIInfoContributor.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_importssrc/main/webapp/app/exercises/programming/manage/update/update-components/custom-build-plans/programming-exercise-build-configuration/programming-exercise-build-configuration.component.html (1)
Pattern
src/main/webapp/**/*.html: @if and @for are new and valid Angular syntax replacing *ngIf and *ngFor. They should always be used over the old style.src/main/webapp/app/exercises/programming/manage/update/update-components/custom-build-plans/programming-exercise-build-configuration/programming-exercise-build-configuration.component.ts (1)
src/main/webapp/app/shared/layouts/profiles/profile-info.model.ts (1)
src/main/webapp/app/shared/layouts/profiles/profile.service.ts (1)
🔇 Additional comments (7)
src/main/webapp/app/exercises/programming/manage/update/update-components/custom-build-plans/programming-exercise-build-configuration/programming-exercise-build-configuration.component.ts (2)
1-9: LGTM! Component follows Angular style guide.The imports are well-organized and the component follows Angular naming conventions.
10-11: LGTM! Modern dependency injection approach.Good use of the new
inject()function for dependency injection, which is more tree-shakable than constructor injection.src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIInfoContributor.java (1)
5-5: LGTM!The Value annotation import is properly added and follows the coding guidelines.
src/main/webapp/app/exercises/programming/manage/update/update-components/custom-build-plans/programming-exercise-build-configuration/programming-exercise-build-configuration.component.html (2)
Line range hint
18-18: LGTM! Correct usage of new Angular control flow syntax.The template correctly uses the new
@ifdirective instead of*ngIf, following the latest Angular guidelines.
Line range hint
31-38: Consider adding validation for timeout values.Based on the PR objectives, there should be specific constraints:
- Minimum: 10 seconds
- Maximum: 360 seconds
- Default: 120 seconds
Consider adding validation to ensure these constraints are enforced in the UI.
Let's verify if these constraints are properly defined in the component:
Would you like me to provide an implementation for the validation logic?
✅ Verification successful
Basic validation exists but can be enhanced for better user experience
The code already implements the basic constraints through HTML5 attributes:
minandmaxattributes are properly bound totimeoutMinValueandtimeoutMaxValue- These values are correctly initialized from profile info with appropriate fallbacks (10s min, 240s max, 120s default)
- The input is marked as
requiredHowever, the validation could be enhanced by:
- Adding visual feedback when values are out of range
- Implementing a custom validator to ensure the value stays within bounds even with direct input
- Adding error messages to guide users when invalid values are entered
Example implementation:
@Directive({ selector: '[timeoutValidator]' }) export class TimeoutValidatorDirective implements Validator { validate(control: AbstractControl): ValidationErrors | null { const value = control.value; if (value < this.timeoutMinValue || value > this.timeoutMaxValue) { return { range: true }; } return null; } }🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if timeout constraints are properly defined in the component # Expected: Find constant definitions or property assignments matching the specified values # Search for timeout-related constant definitions or property assignments rg -A 5 "(timeoutMinValue|timeoutMaxValue|timeoutDefaultValue)" --type tsLength of output: 4069
Script:
#!/bin/bash # Let's check if there are any validation mechanisms in place for the timeout values # Search for validation-related code in the component and its template # Search for validation-related code in the component rg -A 5 "timeoutChange|timeout\(" --type ts src/main/webapp/app/exercises/programming/manage/update/update-components/custom-build-plans/programming-exercise-build-configuration/ # Search for any validation directives in the template rg "min=|max=|required|validate" src/main/webapp/app/exercises/programming/manage/update/update-components/custom-build-plans/programming-exercise-build-configuration/programming-exercise-build-configuration.component.htmlLength of output: 3050
src/main/webapp/app/shared/layouts/profiles/profile-info.model.ts (1)
40-42: LGTM! Properties are well-structured and follow conventions.The new timeout properties are:
- Properly typed as optional numbers
- Follow camelCase naming convention
- Logically grouped with other CI-related properties
- Align with the PR objectives for configurable timeout settings
src/main/webapp/app/shared/layouts/profiles/profile.service.ts (1)
91-93: Verify the integration with ProfileInfo model.Ensure that these new properties are properly defined in the ProfileInfo model with appropriate types.
Let's verify the ProfileInfo model definition:
✅ Verification successful
Based on the gathered information, I can now provide a final response:
The buildTimeout properties are properly defined in the ProfileInfo model
The properties
buildTimeoutMin,buildTimeoutMax, andbuildTimeoutDefaultare correctly defined in the ProfileInfo model (src/main/webapp/app/shared/layouts/profiles/profile-info.model.ts) as optional number properties. These properties are:
- Properly mapped in the profile service from the server response
- Used with appropriate fallback values in the build configuration component:
- buildTimeoutMin: defaults to 10
- buildTimeoutMax: defaults to 240
- buildTimeoutDefault: defaults to 120
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for ProfileInfo model definition rg -t typescript "class ProfileInfo" -A 10Length of output: 79
Script:
#!/bin/bash # Search for ProfileInfo definition using ripgrep without file type restriction rg "ProfileInfo" -A 10Length of output: 303867
Script:
#!/bin/bash # Use fd to find TypeScript files and search for ProfileInfo fd -e ts -x rg "ProfileInfo" -A 10 {}Length of output: 109183
Script:
#!/bin/bash # Search for interface or type definition of ProfileInfo rg "(interface|type) ProfileInfo" -A 10Length of output: 41
Script:
#!/bin/bash # Search for files containing ProfileInfo in their name fd "ProfileInfo"Length of output: 16
Script:
#!/bin/bash # Search for ProfileInfo class definition and its properties rg "class ProfileInfo" -A 30Length of output: 3232
Script:
#!/bin/bash # Search for files containing profile-info in their name fd "profile-info"Length of output: 144
Script:
#!/bin/bash # Search for buildTimeout related properties in any TypeScript files rg "buildTimeout(Min|Max|Default)" -t tsLength of output: 1576
...ogramming-exercise-build-configuration/programming-exercise-build-configuration.component.ts
Outdated
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIInfoContributor.java
Show resolved
Hide resolved
...ramming-exercise-build-configuration/programming-exercise-build-configuration.component.html
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildJobManagementService.java
Show resolved
Hide resolved
HawKhiem
added
deploy:artemis-test4
deploy:artemis-test2
and removed
deploy:artemis-test4
labels
|
github-actions
bot
added
the
deployment-error
HawKhiem
added
deploy:artemis-test4
and removed
deployment-error
HawKhiem
temporarily deployed
to
artemis-test4.artemis.cit.tum.de
GitHub Actions
Inactive
|
github-actions
bot
added
the
deployment-error
SindiBuklaji
added
deploy:artemis-test3
and removed
deployment-error
SindiBuklaji
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
GitHub Actions
Inactive
kevinfischer4
temporarily deployed
to
artemis-test2.artemis.cit.tum.de
GitHub Actions
Inactive
sawys777
temporarily deployed
to
artemis-test1.artemis.cit.tum.de
GitHub Actions
Inactive
krusche
changed the title
Integrated code lifecycle: Allow admins to set timeout options via application propertiesIntegrated code lifecycle: Allow admins to set build timeout options via application properties
krusche
deleted the
chore/integrated-code-lifecycle/change-timeout-options
branch
Require a config change
Checklist
General
Server
Client
authoritiesto all new routes and checked the course groups for displaying navigation elements (links, buttons).Changes affecting Programming Exercises
Motivation and Context
This PR allows admins to adjust the timeout options in the UI.
New properties are added:
artemis.continuous-integration.build-timeout-seconds.min: Min timeout optionartemis.continuous-integration.build-timeout-seconds.max: Max timeout option (if the user somehow sends a value higher than this, it will be checked and overridden)artemis.continuous-integration.build-timeout-seconds.default: default optionSteps for Testing
Prerequisites:
Customize build scriptTestserver 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
Code Review
Manual Tests
Performance Tests
Test Coverage
Client
Screenshots
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests