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

Development: Extract build agent git service #8595

Merged
merged 32 commits into from
May 25, 2024

Conversation

krusche
Copy link
Member

@krusche krusche commented May 14, 2024

Checklist

General

Server

  • Important: I implemented the changes with a very good performance and prevented too many (unnecessary) database calls.
  • I strictly followed the server coding and design guidelines.
  • I added multiple integration tests (Spring) related to the features (with a high test coverage).
  • I documented the Java code using JavaDoc style.

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

Motivation and Context

Currently, there is an inconsistency in build agents when using single and multi-node setup. In multi-node, they use https. In single-node, they copy the folder. This PR fixes this issue and has the added benefit of reducing the responsibilities of GitService

Description

This PR implements several structural and code improvements:

  • Separate GitService and BuildJobGitService and added common functionality in AbstractGitService
  • Adjust the rest of code and tests
  • Change config files so that build agents have the necessary credentials to clone repos from localvc
  • Reduce the amount of services used in build agents and avoid error messages when build agents are started without a cluster
  • Rename and move some classes and packages for more clarity
  • Improve jgit server code

Steps for Testing

Note

Test on both setups! (single node and multi node servers with ICL)

Prerequisites:

  • 1 Students
  • 1 Programming Exercise
  1. Submit exercise and make sure that build runs
  2. Clone repository and make sure that it's cloned correctly
  3. Play around with features related to localvc/ci

Testserver States

Note

These badges show the state of the test servers.
Green = Currently available, Red = Currently locked






Review Progress

Code Review

  • Code Review 1
  • Code Review 2

Manual Tests

  • Test 1
  • Test 2

@krusche krusche self-assigned this May 14, 2024
@github-actions github-actions bot added the server Pull requests that update Java code. (Added Automatically!) label May 14, 2024
# Conflicts:
#	src/main/java/de/tum/in/www1/artemis/service/connectors/GitService.java
coderabbitai[bot]
coderabbitai bot previously approved these changes May 22, 2024
BBesrour added 3 commits May 22, 2024 13:38
…service

# Conflicts:
#	src/main/java/de/tum/in/www1/artemis/service/connectors/GitService.java
#	src/main/java/de/tum/in/www1/artemis/service/connectors/localci/buildagent/BuildJobExecutionService.java
@github-actions github-actions bot added tests config-change Pull requests that change the config in a way that they require a deployment via Ansible. labels May 22, 2024
Copy link
Member

@BBesrour BBesrour 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 TS2 and staging

Copy link
Contributor

@pzdr7 pzdr7 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 LocalCI Staging

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
config-change Pull requests that change the config in a way that they require a deployment via Ansible. docker ready to merge server Pull requests that update Java code. (Added Automatically!) tests
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

8 participants