-
Notifications
You must be signed in to change notification settings - Fork 300
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
: Add Bash programming exercise template
#10089
base: develop
Are you sure you want to change the base?
Conversation
|
|
bb9b2ef
to
7d7ecad
Compare
7d7ecad
to
c48d300
Compare
c48d300
to
1446b6a
Compare
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/domain/ProgrammingLanguage.javaThe 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/TemplateUpgradePolicyService.javaThe 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/ci/ContinuousIntegrationService.javaThe 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.
WalkthroughThis pull request introduces comprehensive support for the Bash programming language across the Artemis platform. The changes span multiple components, including documentation, domain models, services, templates, and frontend configurations. The implementation adds Bash as a fully supported programming language, enabling features like continuous integration, build plan creation, and exercise templates with associated test suites. Changes
Sequence DiagramsequenceDiagram
participant User
participant ArtemisPlatform
participant ContinuousIntegration
participant BuildSystem
User->>ArtemisPlatform: Create Bash Programming Exercise
ArtemisPlatform->>BuildSystem: Configure Build Plan
BuildSystem->>ContinuousIntegration: Prepare CI Environment
ContinuousIntegration->>BuildSystem: Execute Bash Tests
BuildSystem->>ArtemisPlatform: Return Test Results
ArtemisPlatform->>User: Display Exercise Feedback
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
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 6
🧹 Nitpick comments (7)
src/main/resources/templates/bash/readme (2)
7-8
: Consider adding more specific guidance for the shebang task.While the task is clear, it would be helpful to provide examples of standard paths (e.g.,
/bin/bash
or/usr/bin/env bash
) to guide students, especially those new to Bash scripting.
22-24
: Add safety considerations for the find and replace task.The task should mention the importance of handling special characters in the replacement text and suggest using appropriate tools (e.g.,
sed
with proper escaping).src/main/resources/templates/bash/test/test.bats (2)
1-3
: Consider adding cleanup function.While setup is properly handled, there's no cleanup function to restore the test environment. Consider adding a
teardown
function.+teardown() { + cd "$BATS_TEST_DIRNAME" + rm -rf "$BATS_TEST_TMPDIR"/* +}
26-31
: Improve error message clarity.The custom error message could be more descriptive by showing the expected format.
- | batslib_decorate "first line is not a valid shebang" \ + | batslib_decorate "First line must be a valid shebang (e.g., #!/bin/bash or #!/usr/bin/env bash)" \src/main/resources/templates/bash/solution/script.bash (2)
11-11
: Make sed pattern matching more robust.The current sed command might fail with special characters. Consider using a more robust pattern matching approach.
-sed -i 's/2\.718/3.1415/g' numbers.txt +sed -i 's/\b2\.718\b/3.1415/g' numbers.txt
13-13
: Exit with actual operation status.Instead of hardcoding exit 0, consider capturing and using the actual operation status.
-exit 0 +exit $?src/main/resources/templates/aeolus/bash/default.sh (1)
14-17
: Improve test execution error handling.The
|| true
construct silently ignores test failures. Consider capturing and reporting the test status.test () { echo '⚙️ executing test' - bats --report-formatter junit --output results "${testWorkingDirectory}" || true + if ! bats --report-formatter junit --output results "${testWorkingDirectory}"; then + echo "Warning: Tests failed with status $?" + return 1 + fi }🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 16-16: testWorkingDirectory is referenced but not assigned.
(SC2154)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
src/main/resources/config/application.yml
is excluded by!**/*.yml
src/main/resources/templates/aeolus/bash/default.yaml
is excluded by!**/*.yaml
src/test/resources/config/application.yml
is excluded by!**/*.yml
📒 Files selected for processing (18)
docs/user/exercises/programming-exercise-features.inc
(2 hunks)src/main/java/de/tum/cit/aet/artemis/programming/domain/ProgrammingLanguage.java
(1 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/TemplateUpgradePolicyService.java
(1 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/ci/ContinuousIntegrationService.java
(2 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/jenkins/JenkinsProgrammingLanguageFeatureService.java
(2 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/jenkins/build_plan/JenkinsBuildPlanService.java
(1 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIProgrammingLanguageFeatureService.java
(2 hunks)src/main/resources/templates/aeolus/bash/default.sh
(1 hunks)src/main/resources/templates/bash/exercise/script.bash
(1 hunks)src/main/resources/templates/bash/readme
(1 hunks)src/main/resources/templates/bash/solution/script.bash
(1 hunks)src/main/resources/templates/bash/test/test.bats
(1 hunks)src/main/resources/templates/bash/test/test_data/numbers.txt
(1 hunks)src/main/resources/templates/bash/test/test_data/numbers_expected.txt
(1 hunks)src/main/resources/templates/bash/test/test_data/rename_me.txt
(1 hunks)src/main/resources/templates/bash/test/test_helper/common-setup.bash
(1 hunks)src/main/resources/templates/jenkins/bash/regularRuns/pipeline.groovy
(1 hunks)src/main/webapp/app/entities/programming/programming-exercise.model.ts
(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- src/main/resources/templates/bash/test/test_data/rename_me.txt
- src/main/resources/templates/bash/test/test_data/numbers.txt
- src/main/resources/templates/bash/test/test_data/numbers_expected.txt
🧰 Additional context used
📓 Path-based instructions (7)
src/main/webapp/app/entities/programming/programming-exercise.model.ts (1)
src/main/java/de/tum/cit/aet/artemis/programming/service/ci/ContinuousIntegrationService.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/domain/ProgrammingLanguage.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/jenkins/build_plan/JenkinsBuildPlanService.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/TemplateUpgradePolicyService.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/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 (3)
src/main/java/de/tum/cit/aet/artemis/programming/service/TemplateUpgradePolicyService.java (1)
Learnt from: magaupp
PR: ls1intum/Artemis#9074
File: src/main/java/de/tum/in/www1/artemis/service/programming/TemplateUpgradePolicyService.java:35-35
Timestamp: 2024-11-12T12:51:46.554Z
Learning: The `defaultRepositoryUpgradeService` in the `TemplateUpgradePolicyService` is used as a default for all non-JAVA languages, with specialized services added based on testing outcomes if necessary.
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.
🪛 Shellcheck (0.10.0)
src/main/resources/templates/bash/test/test_helper/common-setup.bash
[warning] 7-7: studentParentWorkingDirectoryName is referenced but not assigned.
(SC2154)
src/main/resources/templates/aeolus/bash/default.sh
[warning] 6-6: studentParentWorkingDirectoryName is referenced but not assigned.
(SC2154)
[warning] 16-16: testWorkingDirectory is referenced but not assigned.
(SC2154)
🔇 Additional comments (14)
src/main/resources/templates/jenkins/bash/regularRuns/pipeline.groovy (1)
31-33
: Consider restricting file permissions.The current implementation grants execute permissions to all files in the assignment directory. Consider being more selective about which files need execute permissions.
✅ Verification successful
Current implementation is appropriate for CI environment
The blanket execute permissions are acceptable in this context because:
- Files are in an isolated Docker container
- Directory is created fresh for each build
- Permissions are required for build and test execution
- Scope is limited to the assignment directory only
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for potentially sensitive files that might get execute permissions find ./assignment -type f -not -name "*.sh" -not -name "*.bash" | grep -v "test"Length of output: 128
Script:
#!/bin/bash # Search for documentation and configuration files fd -e md -e yaml -e yml -e json # Search for patterns defining assignment structure rg -i "assignment.*directory|directory.*structure" -A 3 -B 3 # Find similar pipeline configurations fd pipeline.groovyLength of output: 67657
src/main/java/de/tum/cit/aet/artemis/programming/service/TemplateUpgradePolicyService.java (1)
35-37
: LGTM! Changes align with existing patterns.The addition of BASH to the supported languages using
defaultRepositoryUpgradeService
follows the established pattern for non-JAVA languages, as confirmed by previous learnings.src/main/java/de/tum/cit/aet/artemis/programming/domain/ProgrammingLanguage.java (1)
42-42
: LGTM! BASH language support added correctly.The addition of BASH to the ENABLED_LANGUAGES set is properly placed and follows the alphabetical ordering convention.
src/main/java/de/tum/cit/aet/artemis/programming/service/jenkins/JenkinsProgrammingLanguageFeatureService.java (2)
42-42
: LGTM! BASH configuration aligns with other scripting languages.The BASH configuration follows the same pattern as other scripting languages like Python, with appropriate feature flags set.
49-51
: Verify the feature flag changes for Java and Kotlin.The last parameter for both JAVA and KOTLIN has been changed to
false
. Please confirm if this is intentional as it might affect existing functionality.✅ Verification successful
The feature flag changes for Java and Kotlin are consistent with the codebase.
The changes only affect the auxiliary repositories support (last parameter), which is now set to
false
for both languages. The static code analysis settings remain unchanged:
- Java: Static code analysis remains enabled (true)
- Kotlin: Static code analysis remains disabled (false)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any related configuration or documentation about Java/Kotlin feature flags rg -l "JAVA|KOTLIN.*feature" docs/Length of output: 131
Script:
#!/bin/bash # Check documentation content cat docs/dev/setup/server.rst docs/dev/setup/jenkins-localvc.rst docs/dev/setup/jenkins-gitlab.rst | rg -A 5 -B 5 "JAVA|KOTLIN.*feature" # Find and examine ProgrammingLanguageFeature class definition fd -e java ProgrammingLanguageFeatureLength of output: 2269
Script:
#!/bin/bash # Check ProgrammingLanguageFeature class definition cat src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingLanguageFeature.java # Search for references to this feature in the codebase rg -A 5 -B 5 "isStaticCodeAnalysisEnabled|staticCodeAnalysis" --type javaLength of output: 73495
src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIProgrammingLanguageFeatureService.java (1)
47-65
: Verify the consistent feature flag changes across all languages.All languages now have their last feature flag set to
true
. This appears to be a systematic change that affects all languages, including the newly added BASH.✅ Verification successful
Auxiliary repository support is intentionally enabled only in LocalCI
The consistent
true
values for auxiliary repository support in LocalCI are intentionally different from other CI implementations (Jenkins, GitLabCI) where it's consistently set tofalse
. This represents a legitimate feature capability difference between CI systems, not an inconsistency that needs to be fixed.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any documentation or commits explaining this feature flag change rg -l "LocalCI.*feature.*true" docs/ git log -p --all -S "LocalCI.*feature.*true" -- "*.md" "*.adoc"Length of output: 38
Script:
#!/bin/bash # Find ProgrammingLanguageFeature class definition ast-grep --pattern 'class ProgrammingLanguageFeature { $$$ }' # Find any test files related to this class fd -e java -e kt test --exec rg -l "ProgrammingLanguageFeature" # Look for any configuration or constant files that might explain these features rg -l "ProgrammingLanguageFeature" -g "!*.{class,jar}"Length of output: 2819
Script:
#!/bin/bash # Get the ProgrammingLanguageFeature class definition cat src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingLanguageFeature.java # Check other CI implementations for consistency rg -A 5 -B 5 "ProgrammingLanguageFeature\(" src/main/java/de/tum/cit/aet/artemis/programming/service/jenkins/JenkinsProgrammingLanguageFeatureService.java src/main/java/de/tum/cit/aet/artemis/programming/service/gitlabci/GitLabCIProgrammingLanguageFeatureService.javaLength of output: 8378
src/main/webapp/app/entities/programming/programming-exercise.model.ts (1)
17-17
: LGTM! BASH enum value added correctly.The addition of BASH to the ProgrammingLanguage enum is consistent with the backend changes and follows proper alphabetical ordering.
docs/user/exercises/programming-exercise-features.inc (1)
50-51
: LGTM! Documentation changes are accurate and well-structured.The tables correctly document Bash support in both CI systems and accurately reflect the current feature limitations.
Also applies to: 96-97
src/main/resources/templates/bash/test/test_helper/common-setup.bash (3)
24-30
: LGTM!The function is well-documented and correctly implements file content comparison using
diff
. The error handling is appropriate.
33-33
: LGTM!The function is correctly implemented as a no-op to reduce output during test execution.
34-34
: LGTM!The function is correctly implemented as a no-op to reduce output during test execution.
src/main/java/de/tum/cit/aet/artemis/programming/service/ci/ContinuousIntegrationService.java (2)
222-223
: LGTM!The addition of BASH to the list of languages that return "assignment" is consistent with the PR objectives and follows the established pattern.
233-234
: LGTM!The addition of BASH to the list of languages that return "tests" is consistent with the PR objectives and follows the established pattern.
src/main/java/de/tum/cit/aet/artemis/programming/service/jenkins/build_plan/JenkinsBuildPlanService.java (1)
188-189
: LGTM!The addition of BASH to the list of languages that use jenkinsBuildPlanCreator is consistent with the PR objectives and follows the established pattern.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really nice addition 👍 (Code)
Compact and concise template exercise
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested on TS3. The builds run through without errors, and the feedback of the template result also makes sense.
Checklist
General
Server
Changes affecting Programming Exercises
Description
The PR adds a programming exercise template for Bash.
The testing framework in use is Bats.
Steps for Testing
Prerequisites:
Bash
as the programming languageTestserver 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
Manual Tests
Summary by CodeRabbit
New Features
Documentation
Chores