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

Programming exercises: Fix programming language feature flags #10110

Conversation

magaupp
Copy link
Contributor

@magaupp magaupp commented Jan 6, 2025

Checklist

General

Server

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).
  • I tested all changes and their related features with all corresponding user types on a local server configured with LocalVC and Jenkins.

Motivation and Context

#9993 removed testwise coverage support and its testwiseCoverageAnalysisSupported flag from the ProgrammingLanguageFeature record.
The ProgrammingLanguageFeatureService implementations were incorrectly updated to remove the last parameter to the ProgrammingLanguageFeature record (auxiliaryRepositoriesSupported), while the testwiseCoverageAnalysisSupported parameter is the second to last.
The recent addition of Python static code analysis was also reverted.

As a result auxiliary repository support on LocalCI was mostly disabled. On Jenkins auxiliary repository support was enabled where it was not supported.

Description

This PR restores the supported features as they were before.

Steps for Testing

Prerequisites:

  • 1 Instructor
  • Server configured with LocalCI
  1. Navigate to the programming exercise creation page
  2. Switch to advanced mode using the button on the bottom left
  3. Enter a valid "short name"
  4. Below the repository preview an "Add Auxiliary Repository" button should be present
    grafik
  5. Select "Python" as the programming language
  6. A checkbox for "Enable Static Code Analysis" should be present
    grafik

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

  • Code Review

Manual Tests

  • Test

Summary by CodeRabbit

  • Configuration Changes
    • Updated programming language feature settings for Java and Kotlin in Jenkins service
    • Enabled features for multiple programming languages in Local CI service, including Java, Kotlin, Python, and others

@github-actions github-actions bot added server Pull requests that update Java code. (Added Automatically!) programming Pull requests that affect the corresponding module labels Jan 6, 2025
@magaupp magaupp temporarily deployed to artemis-test2.artemis.cit.tum.de January 6, 2025 15:00 — with GitHub Actions Inactive
@magaupp magaupp marked this pull request as ready for review January 6, 2025 15:04
@magaupp magaupp requested a review from a team as a code owner January 6, 2025 15:04
@magaupp magaupp requested a review from ole-ve January 6, 2025 15:06
Copy link

coderabbitai bot commented Jan 6, 2025

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.8.0)
src/main/java/de/tum/cit/aet/artemis/programming/service/jenkins/JenkinsProgrammingLanguageFeatureService.java

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

src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIProgrammingLanguageFeatureService.java

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

Walkthrough

The pull request involves modifications to two programming language feature services: JenkinsProgrammingLanguageFeatureService and LocalCIProgrammingLanguageFeatureService. In the Jenkins service, the JAVA and KOTLIN programming language features have been modified by changing their last constructor parameter from true to false. Conversely, in the Local CI service, multiple programming languages have had their feature parameters changed from false to true, enabling features across a wide range of languages.

Changes

File Change Summary
src/main/java/de/tum/cit/aet/artemis/programming/service/jenkins/JenkinsProgrammingLanguageFeatureService.java Modified ProgrammingLanguageFeature constructor parameters for JAVA and KOTLIN from true to false
src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIProgrammingLanguageFeatureService.java Updated feature configurations for multiple programming languages, changing last constructor parameter from false to true for languages like JAVA, KOTLIN, C, PYTHON, etc.

Possibly related PRs

Suggested labels

bugfix, ready for review

Suggested reviewers

  • SimonEntholzer
  • JohannesStoehr
  • BBesrour
  • edkaya

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
🪧 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 generate docstrings to generate docstrings for this PR. (Beta)
  • @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.

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.

Actionable comments posted: 0

🧹 Nitpick comments (1)
src/main/java/de/tum/cit/aet/artemis/programming/service/jenkins/JenkinsProgrammingLanguageFeatureService.java (1)

49-49: Check consistency for KOTLIN feature flags.

Similar to JAVA, ensure that the newly set false aligns with the intended support level for Kotlin in Jenkins. This setting can help avoid confusion if other parts of the codebase expect auxiliary repository support to be enabled.

You may wish to unify the logic for enabling and disabling auxiliary repositories by referencing a shared configuration or feature toggle if this setting is frequently changed.

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1798b25 and 3c3593b.

📒 Files selected for processing (2)
  • src/main/java/de/tum/cit/aet/artemis/programming/service/jenkins/JenkinsProgrammingLanguageFeatureService.java (1 hunks)
  • src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIProgrammingLanguageFeatureService.java (1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/main/java/de/tum/cit/aet/artemis/programming/service/jenkins/JenkinsProgrammingLanguageFeatureService.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/localci/LocalCIProgrammingLanguageFeatureService.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

📓 Learnings (2)
src/main/java/de/tum/cit/aet/artemis/programming/service/jenkins/JenkinsProgrammingLanguageFeatureService.java (2)
Learnt from: magaupp
PR: ls1intum/Artemis#9256
File: src/main/java/de/tum/cit/aet/artemis/programming/service/jenkins/JenkinsProgrammingLanguageFeatureService.java:0-0
Timestamp: 2024-11-12T12:52:03.805Z
Learning: In `JenkinsProgrammingLanguageFeatureService.java`, comments explaining boolean flags in calls to the `ProgrammingLanguageFeature` constructor are unnecessary because the parameter names are clear, and most IDEs display them as inline hints.
Learnt from: magaupp
PR: ls1intum/Artemis#8802
File: src/main/java/de/tum/in/www1/artemis/service/connectors/jenkins/JenkinsProgrammingLanguageFeatureService.java:43-43
Timestamp: 2024-11-12T12:51:58.050Z
Learning: Jenkins support for Rust was added in commit d0fb7032e70c001dc8b2c4d0a391ca8bf7e7d586.
src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIProgrammingLanguageFeatureService.java (1)
Learnt from: magaupp
PR: ls1intum/Artemis#9261
File: src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIProgrammingLanguageFeatureService.java:43-55
Timestamp: 2024-11-12T12:51:40.391Z
Learning: For constructors with multiple boolean parameters, it's acceptable to keep them as is because parameter names are clear and IDEs provide inline hints, making the code readable without refactoring to the builder pattern.
🔇 Additional comments (2)
src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIProgrammingLanguageFeatureService.java (1)

46-63: Confirm reintroduction of auxiliaryRepositoriesSupported for all languages.

All these lines set the last constructor parameter to true, re-enabling auxiliary repository support for each language. This change aligns with the PR goal to revert and restore the previous feature flags. However, please verify each language’s settings to ensure the flags accurately reflect the intended behavior. If some of these languages are not supposed to support auxiliary repositories, consider reverting them to false.

src/main/java/de/tum/cit/aet/artemis/programming/service/jenkins/JenkinsProgrammingLanguageFeatureService.java (1)

47-47: Confirm auxiliary repository support setting for JAVA.

The last parameter presumably indicates auxiliary repository support (or a similar feature). Verify that false is correct for Jenkins, aligning with the PR objective to disable auxiliary repositories for this service.

Run the following command to confirm no contradictions with older definitions or calls that assume auxiliary repository support for Java on Jenkins:

✅ Verification successful

Auxiliary repository support correctly disabled for Java in Jenkins service

The verification confirms that setting auxiliaryRepositoriesSupported to false for Java is correct and consistent with all other languages in the Jenkins service implementation. This aligns with the PR objective to disable auxiliary repositories for this service.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for references to auxiliaryRepositoriesSupported in Jenkins contexts for Java
rg -A 5 'programmingLanguageFeatures\.put\(JAVA.+auxiliaryRepositoriesSupported'

Length of output: 82


Script:

#!/bin/bash
# Find and show ProgrammingLanguageFeature class definition
ast-grep --pattern 'class ProgrammingLanguageFeature {
  $$$
}'

# Search for auxiliary repository related code in Jenkins context
rg -i "auxiliary.*repository.*jenkins" -A 3

# Search for Java configurations in Jenkins services
rg "JAVA.*Jenkins" -A 3

Length of output: 1794


Script:

#!/bin/bash
# Search for auxiliary repository references in Jenkins service files
rg -g "**/*Jenkins*.java" -i "auxiliary.*repository" -A 3

# Find ProgrammingLanguageFeature usage in Jenkins context
rg -g "**/*Jenkins*.java" "ProgrammingLanguageFeature" -A 3

# Search for configuration properties related to auxiliary repositories
rg -g "**/*.properties" -g "**/*.yml" -i "auxiliary.*repository"

Length of output: 10669

Copy link
Contributor

@ole-ve ole-ve 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

@HawKhiem HawKhiem left a comment

Choose a reason for hiding this comment

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

Tested on TS3. Works as described

image

image

Copy link

@ahbitaqu ahbitaqu left a comment

Choose a reason for hiding this comment

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

[Tested on TS3]
Works as decribed!

QQ: Is there supposed to be a checkbox in this view as well, since there is one for other options
image

Copy link
Contributor

@chrisknedl chrisknedl left a comment

Choose a reason for hiding this comment

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

Tested on my local setup. It's all there 👍
image

@magaupp
Copy link
Contributor Author

magaupp commented Jan 9, 2025

Thank you for the reviews.

@ahbitaqu There hasn't been a checkmark for SCA in the Detail view before, but I can easily add one

@krusche
Copy link
Member

krusche commented Jan 9, 2025

Thanks for the fix 👍

@krusche krusche added this to the 7.8.3 milestone Jan 9, 2025
@krusche krusche merged commit 599c92e into develop Jan 9, 2025
89 of 91 checks passed
@krusche krusche deleted the bugfix/programming-exercises/fix-programming-language-feature-flags branch January 9, 2025 18:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
programming Pull requests that affect the corresponding module ready to merge server Pull requests that update Java code. (Added Automatically!)
Projects
Status: Merged
Status: Done
Development

Successfully merging this pull request may close these issues.

6 participants