Artemis
Artemis copied to clipboard
`Development`: Refactor configuration of CI Docker images
Checklist
General
- [ ] I tested all changes and their related features with all corresponding user types on a test server.
- [x] This is a small issue that I tested locally and was confirmed by another developer on a test server.
- [x] I chose a title conforming to the naming conventions for pull requests.
Server
- [x] I followed the coding and design guidelines.
- [x] I added multiple integration tests (Spring) related to the features (with a high test coverage).
- [ ] I added
@PreAuthorize
and checked the course groups for all new REST Calls (security). - [x] I implemented the changes with a good performance and prevented too many database calls.
- [x] I documented the Java code using JavaDoc style.
Motivation and Context
#5295 and #5594:
The current loader for the programming language specific image configuration does not handle lowercase names for programming languages.
When converting the image config of C with the FACT-framework to the ENV representation, the underscore in C_FACT
has to be removed.
A fix for those two issues is required so that a configuration via environment variables can work.
Description
- The programming languages themself are now case-independent.
- For each programming language a map with different project types can be configured instead of making the project type part of the programming language name. Therefore, the underscore in the config option is no longer needed.
- This also makes the configuration easier to extend with new project types in the future.
Steps for Testing
Test Servers
- Create two new programming exercises, one with language C+GCC and one with C+FACT.
- The builds for the template and solution should pass/fail as usual.
Local Testing
- In general, try to play around with the config options by removing the default, adding new languages, and/or removing languages.
- All of those cases should result in an invalid configuration, thereby aborting the server startup.
- You can remove values from the config and instead replace them by environment variables (configurable in the Run/Debug Configurations -> Modify Options). E.g.,
ARTEMIS_CONTINUOUSINTEGRATION_BUILD_IMAGES_JAVA_DEFAULT="ls1tum/artemis-maven-template:java17-8"
- Spring merges the information from the yaml and the environment. Therefore, the server should start up without errors.
Review Progress
Code Review
- [ ] Review 1
- [ ] Review 2
Manual Tests
- [ ] Test 1
- [ ] Test 2
Test Coverage
Class/File | Branch | Line |
---|---|---|
ProgrammingLanguageConfiguration |
100% | 100% |
Starting the branch out-of-the-box currently produces an error because of java.other: "asd"
in application.yml
which I suppose is just some testing residue. Although, the problem is the somehow misleading log referring to the ocaml.default
image which isn't responsible for the error in this case.
***************************
APPLICATION FAILED TO START
***************************
Description:
Failed to bind properties under 'artemis.continuous-integration.build' to de.tum.in.www1.artemis.config.ProgrammingLanguageConfiguration:
Property: artemis.continuous-integration.build.images.ocaml.default
Value: "ls1tum/artemis-ocaml-docker:v1"
Origin: class path resource [config/application.yml] - 65:30
Reason: java.lang.IllegalArgumentException: Unknown project type: other
Apart from that, everything working fine (tested locally) 🚀
Although, the problem is the somehow misleading log referring to the ocaml.default image which isn't responsible for the error in this case.
@daniels98it That is because the whole part of the config under build.images is read a map at once. Spring then seems to report the error location as the ending of the map. I now at least improved the Reason
as
Reason: java.lang.IllegalArgumentException: Unknown project type for JAVA: other
That should hopefully be clear enough.
@b-fein I'll test it again asap
Although, the problem is the somehow misleading log referring to the ocaml.default image which isn't responsible for the error in this case.
@daniels98it That is because the whole part of the config under build.images is read a map at once. Spring then seems to report the error location as the ending of the map. I now at least improved the
Reason
asReason: java.lang.IllegalArgumentException: Unknown project type for JAVA: other
That should hopefully be clear enough.
Retested different wrong configs as I promised to do so. I couldn't get the language
inside the printed error message as you wrote in the comment above for the example for java.other="asd"
.
It seems like it's catching the error in tryParseProjectType
but instead of consuming it forwards the caught error, which is without the language
inside of the message.
What I don't get is why the test with the wrong project type is not failing ... Maybe this has to do with the surrounding errors of the loop not existing or being different in the tests.
@4ludwig4 Thanks for the review. Apparently Spring does some kind of root-cause-search for the errors and just takes the first exception within application code (?). Fixed in 3f35dc4faa30e50bf01f4aafcfc99245b2b05861 by intentionally not producing a stack trace that can be filtered like that.