Artemis icon indicating copy to clipboard operation
Artemis copied to clipboard

`Development`: Refactor configuration of CI Docker images

Open b-fein opened this issue 2 years ago • 2 comments

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

  1. Create two new programming exercises, one with language C+GCC and one with C+FACT.
  2. 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%

b-fein avatar Aug 10 '22 15:08 b-fein

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) 🚀

daniels98it avatar Aug 11 '22 08:08 daniels98it

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 avatar Aug 11 '22 14:08 b-fein

@b-fein I'll test it again asap

dfuchss avatar Aug 18 '22 20:08 dfuchss

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.

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.

Screenshot from 2022-08-22 15-29-13

4ludwig4 avatar Aug 22 '22 13:08 4ludwig4

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

b-fein avatar Aug 22 '22 14:08 b-fein