ls1intum / Artemis

Artemis - Interactive Learning with Automated Feedback
https://docs.artemis.cit.tum.de
MIT License
519 stars 297 forks source link

`Programming exercises`: Use build plan editor view for all CI systems (especially LocalCI) #8994

Open b-fein opened 4 months ago

b-fein commented 4 months ago

Is your feature request related to a problem?

In Artemis instances connected to GitLab-CI or Jenkins as CI systems, there is a special view related to editing the build script (‘Edit build plan’ in the screenshot below).

I think it would make sense to also use this when Artemis is connected to LocalCI. This would

image image

Describe the solution you'd like

Right now the build plan editor page only shows an editor where you can change the build script, and the status of the template/solution builds above it.

When integrating the editor for LocalCI into this page, it should also allow to select the container image, and should allow to select the steps on the left like it is currently the case when editing the programming exercise itself.

Describe alternatives you've considered

No response

Additional context

Related issue: #8887, adds more options to the build plan for LocalCI, thus making the configuration page for programming exercises even more complex if the settings are not moved to the separate page.

cc @BBesrour Since you might want to keep this in mind when working on #8887.

krusche commented 4 months ago

@pzdr7 could you help us with this issue when you find some time?

pzdr7 commented 4 months ago

I think so 👍 I'd suggest that we get #8978 ready to merge (so that all the build plan editors already work with Monaco) and then start moving the relevant elements to the "Edit build plan" view

krusche commented 4 months ago

I see two issues here:

  1. It's more intuitive to edit the build plan like it is done in LocalCI with the "customize" during create and edit operations
  2. LocalCI does not follow the existing "OneToMany" relationship between Jenkins build plans and programming exercises. As it is an edge case to customize the build plan, I don't think it's actually necessary that instructors can edit one build plan that is used for multiple programming exercises. Also, in case they want to deviate from this, it might be cumbersome to achieve that (or not intuitive)

I generally agree that it would be ideal to unify this. We will create a new build config object with a OneToOne relationship from programming exercises to bundle all build specific adaptions. If it is fine for you to store Jenkins build plans here as well, we could consider that. However the OneToMany Relationship would then be lost and we would bundle this in the existing customize dialog, potentially offering a bulk edit (like it is already available) in the future

b-fein commented 4 months ago

LocalCI does not follow the existing "OneToMany" relationship between Jenkins build plans and programming exercises.

This OneToMany relation was only introduced on a database-level to save space by not storing duplicated identical build plans.

I don't think it's actually necessary that instructors can edit one build plan that is used for multiple programming exercises.

When editing a build plan, you always only edit the build plan for an individual exercise. If necessary, the changed build plan will be created as new one in the database and only linked to this exercise. The other ones remain untouched.

https://github.com/ls1intum/Artemis/blob/fbc7e14b127c563b6011ccb20287714b06e33de2/src/main/java/de/tum/in/www1/artemis/repository/BuildPlanRepository.java#L49

Therefore, I don’t see a problem with turning the relation from programming exercise to build plan into a one-to-one relation[^1]. The space-saving is not necessary either, I think, since it at most saves in the order of kilobytes across the whole database.


Related issue for the original design for GitLab CI: #6099

[^1]: except possibly the usual issue that lazy-loading does not work for one-to-one relations.

krusche commented 4 months ago

Ok, then we would consider to unify this. It's possible to create a OneToOne relationship that is fetched lazily, see

https://docs.artemis.cit.tum.de/dev/guidelines/database.html#relationships

@BBesrour could you double check if we can merge the build plan table and the build script column into the new build config table that you are currently working on? Ideally we can do this with a liquibase migration. If this would be too difficult, a Java migration would be fine as well.

The use of both is mutually exclusive:

FYI: the build config should allow further customization, such as docker flags, timeout, branch regex, etc. @BBesrour specified this in https://github.com/ls1intum/Artemis/issues/8887

In most cases those settings are not needed so we hide it in a lazy relationship and only fetch it in the cases it's needed (e.g. editing programming exercises, executing the build, etc)