Skip to content
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: Add pre-generated bash scripts for LocalCI #7811

Merged
merged 78 commits into from
Jan 12, 2024

Conversation

reschandreas
Copy link
Contributor

@reschandreas reschandreas commented Dec 20, 2023

Checklist

General

Server

  • Important: I implemented the changes with a very good performance and prevented too many (unnecessary) database calls.
  • I followed the 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 followed the 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 added multiple screenshots/screencasts of my UI changes.
  • I translated all newly inserted strings into English and German.

Motivation and Context

In order to remove hardcoded scripts within the Java code, we add the generated bash scripts of all aeolus templates to the resources. This allows us to have a good baseline for build scripts.
Now the UI changes a bit for Programming Exercises, if you start Artemis with the profile aeolus, you will be greeted with the existing UI. With localci as a profile, the UI changes to show a single code editor that contains the generated build plan.

Description

I added the class BuildScriptProvider and the abstract class BuildScriptGenerationService. The AeolusBuildScriptGenerationService extends the abstract service to provide a code generation from the windfile. If Aeolus is not active, the BuildScriptProvider will provide the pregenerated bash scripts added by this PR.
In addition to the new classes, we no cache all scripts on the startup of Artemis.

The bash scripts are generated using the new bash script supporting_scripts/generate-aeolus-scripts.sh using Aeolus. To ensure that the windfiles and bash scripts always up-to-date, I added a new workflow that checks and complains if the source and the generated content does not match.

Database revert

In the unlikely case my database change makes problems, use alter table programming_exercise_details drop column build_script; to revert it

Steps for Testing

Prerequisites:

  • 1 Instructor
  • LocalCI setup or Aeolus setup
  1. Log in to Artemis
  2. Create a Programming exercise in every language and confirm that the build job works as before.

Testserver States

Note

These badges show the state of the test servers.
Green = Currently available, Red = Currently locked







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

Test Coverage

Client

Class/File Line Coverage Confirmation (assert/expect)
programming-exercise.model.ts 100%
programming-exercise-update.component.ts 87.88%
programming-exercise-custom-build-plan.component.ts 94.23%
programming-exercise-docker-image.component.ts 100%
programming-exercise-language.component.ts 100%
aeolus.service.ts 100%

Server

Class/File Line Coverage Confirmation (assert/expect)
AeolusTarget.java 100%
ProfileService.java 100%
BuildScriptGenerationService.java 100%
AeolusBuildPlanService.java 87%
AeolusTemplateService.java 92%

Screenshots

Screenshot 2023-12-20 at 20 48 30

Summary by CodeRabbit

  • New Features

    • Introduced a new configuration for YAML files in the editor settings.
    • Added a buildScript attribute for programming exercises to handle build configurations.
    • Introduced new constants and services for build script generation and management.
    • Expanded programming exercise features to support custom build plans and Docker image management.
  • Enhancements

    • Improved code modularity in ProfileService with refactored methods for profile checks.
    • Enhanced ProgrammingExerciseService with integration of BuildScriptGenerationService.
    • Updated web components to support new programming exercise configurations.
  • Bug Fixes

    • Fixed issues related to conditional rendering in programming exercise language components.
  • Documentation

    • Updated inline documentation to reflect new features and changes.
  • Tests

    • Added and modified test cases to cover new functionality and profile configurations.
  • Chores

    • Created supporting scripts for automated script generation.
    • Set up GitHub Actions workflow for consistency checks on scripts.
  • Refactor

    • Refactored existing components to integrate new build plan customization options.
  • Style

    • Adjusted styles and templates to accommodate new programming exercise attributes.
  • Revert

    • No reverts in this release.

@github-actions github-actions bot added tests server Pull requests that update Java code. (Added Automatically!) client Pull requests that update TypeScript code. (Added Automatically!) labels Dec 20, 2023
@github-actions github-actions bot added database Pull requests that update the database. (Added Automatically!). Require a CRITICAL deployment. template labels Dec 20, 2023
@reschandreas reschandreas marked this pull request as ready for review December 20, 2023 17:07
@reschandreas reschandreas requested a review from a team as a code owner December 20, 2023 17:07
mateusmm01
mateusmm01 previously approved these changes Jan 9, 2024
Copy link
Contributor

@mateusmm01 mateusmm01 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New code changes lgtm

Copy link

@coderabbitai coderabbitai bot left a 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: 1

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between 5a94bc6 and 0c3a812.
Files ignored due to filter (1)
  • src/main/resources/config/liquibase/master.xml
Files selected for processing (2)
  • src/main/java/de/tum/in/www1/artemis/service/connectors/GenericBuildScriptGenerationService.java (1 hunks)
  • src/main/java/de/tum/in/www1/artemis/service/connectors/aeolus/AeolusBuildScriptGenerationService.java (1 hunks)
Additional comments: 5
src/main/java/de/tum/in/www1/artemis/service/connectors/GenericBuildScriptGenerationService.java (2)
  • 16-17: The @Profile annotation specifies that this service is active when the aeolus profile is not active and the localci profile is. This is consistent with the PR's objective to support LocalCI.

  • 27-28: The constructor is correctly using dependency injection to obtain a BuildScriptProvider. This follows the best practice of constructor injection for dependencies.

src/main/java/de/tum/in/www1/artemis/service/connectors/aeolus/AeolusBuildScriptGenerationService.java (3)
  • 15-17: The @Profile annotation specifies that this service is active when both aeolus and localci profiles are active. This seems to be a change from the previous version and should be verified to ensure it aligns with the intended behavior.
Verification successful

The review comment is correct. The AeolusBuildScriptGenerationService is intended to be active only when both aeolus and localci profiles are active, as confirmed by the @Profile("aeolus && localci") annotation in the codebase.

Scripts Executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Verify that the AeolusBuildScriptGenerationService is intended to be active only when both aeolus and localci profiles are active.
grep -R '@Profile' --include=*.java

Length of output: 19395

* 33-38: The constructor is correctly using dependency injection for `BuildScriptProvider`, `AeolusBuildPlanService`, `AeolusTemplateService`, and `ProfileService`. This follows the best practice of constructor injection for dependencies.
  • 41-56: The getScript method now includes logic to handle the case when the localci profile is not active by returning null. It also handles the case when a windfile is not present by attempting to get a default one. The previous comment about returning null still stands, and it would be better to throw a custom exception instead of returning null.
- return null;
+ throw new BuildScriptNotAvailableException("Local CI profile is not active or windfile is not present.");

Copy link

@coderabbitai coderabbitai bot left a 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

Commits Files that changed from the base of the PR and between 0c3a812 and 9fd2629.
Files selected for processing (1)
  • src/main/java/de/tum/in/www1/artemis/service/connectors/aeolus/AeolusBuildScriptGenerationService.java (1 hunks)
Files skipped from review as they are similar to previous changes (1)
  • src/main/java/de/tum/in/www1/artemis/service/connectors/aeolus/AeolusBuildScriptGenerationService.java

# Conflicts:
#	src/main/java/de/tum/in/www1/artemis/service/ProfileService.java
Copy link

@coderabbitai coderabbitai bot left a 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: 2

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between 9fd2629 and cb98336.
Files selected for processing (2)
  • src/main/java/de/tum/in/www1/artemis/service/ProfileService.java (2 hunks)
  • src/main/java/de/tum/in/www1/artemis/service/connectors/aeolus/AeolusBuildPlanService.java (5 hunks)
Additional comments: 4
src/main/java/de/tum/in/www1/artemis/service/ProfileService.java (1)
  • 25-25: The refactoring of isLocalVcsCi() to use the new methods isLocalVcs() and isLocalCi() is a good example of clean code and improves readability.
src/main/java/de/tum/in/www1/artemis/service/connectors/aeolus/AeolusBuildPlanService.java (3)
  • 19-20: The addition of imports for LinkedMultiValueMap and MultiValueMap is necessary for the new generateBuildScript method. Ensure that these classes are used appropriately within the method.

  • 43-43: Renaming the LOGGER variable to log aligns with the common Java logging convention and improves readability.

  • 92-92: The error handling in the publishBuildPlan method has been updated to use the log variable. Ensure that the logging statements provide enough context for debugging in case of errors.

Also applies to: 111-112

@reschandreas reschandreas requested a review from Mtze January 10, 2024 08:23
Copy link

@coderabbitai coderabbitai bot left a 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

Commits Files that changed from the base of the PR and between cb98336 and 5f65661.
Files selected for processing (1)
  • src/test/java/de/tum/in/www1/artemis/AbstractSpringIntegrationLocalCILocalVCTest.java (1 hunks)
Files skipped from review due to trivial changes (1)
  • src/test/java/de/tum/in/www1/artemis/AbstractSpringIntegrationLocalCILocalVCTest.java

Copy link
Member

@Mtze Mtze left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes are looking good to me - Thanks for taking care!

@Mtze Mtze added maintainer-approved The feature maintainer has approved the PR and removed ready for review labels Jan 10, 2024
Copy link
Contributor

@milljoniaer milljoniaer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

approve after code changes 🚀

Copy link
Contributor

@laurenzfb laurenzfb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changes look good

Copy link
Contributor

@mateusmm01 mateusmm01 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

code changes lgtm

Copy link
Contributor

@basak-akan basak-akan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

code lgtm

@krusche krusche added this to the 6.7.5 milestone Jan 12, 2024
@krusche krusche merged commit a9980af into develop Jan 12, 2024
37 of 48 checks passed
@krusche krusche deleted the aeolus-integration branch January 12, 2024 13:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client Pull requests that update TypeScript code. (Added Automatically!) database Pull requests that update the database. (Added Automatically!). Require a CRITICAL deployment. maintainer-approved The feature maintainer has approved the PR ready to merge server Pull requests that update Java code. (Added Automatically!) tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants