From 08b22d38382c88217c1b7fdfb8b36584d94b842f Mon Sep 17 00:00:00 2001 From: Mohamed Bilel Besrour Date: Wed, 16 Oct 2024 19:04:16 +0200 Subject: [PATCH] coderabbit feedback --- .../service/BuildJobContainerService.java | 27 ++++++++++--------- .../service/BuildJobExecutionService.java | 2 +- .../ProgrammingExerciseBuildConfig.java | 4 +-- .../ci/ContinuousIntegrationService.java | 1 + ...-exercise-build-configuration.component.ts | 5 +--- .../icl/LocalVCLocalCIIntegrationTest.java | 2 +- 6 files changed, 20 insertions(+), 21 deletions(-) diff --git a/src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildJobContainerService.java b/src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildJobContainerService.java index 52d8d46f01e1..bb6157ad5073 100644 --- a/src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildJobContainerService.java +++ b/src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildJobContainerService.java @@ -33,7 +33,6 @@ import com.github.dockerjava.api.DockerClient; import com.github.dockerjava.api.async.ResultCallback; -import com.github.dockerjava.api.command.CreateContainerCmd; import com.github.dockerjava.api.command.CreateContainerResponse; import com.github.dockerjava.api.command.ExecCreateCmd; import com.github.dockerjava.api.command.ExecCreateCmdResponse; @@ -102,21 +101,18 @@ public CreateContainerResponse configureContainer(String containerName, String i envVars.add("NO_PROXY=" + noProxy); } envVars.add("SCRIPT=" + buildScript); - CreateContainerCmd createContainerCmd = dockerClient.createContainerCmd(image).withName(containerName).withHostConfig(hostConfig).withEnv(envVars) + if (exerciseEnvVars != null && !exerciseEnvVars.isEmpty()) { + envVars.addAll(exerciseEnvVars); + } + return dockerClient.createContainerCmd(image).withName(containerName).withHostConfig(hostConfig).withEnv(envVars) // Command to run when the container starts. This is the command that will be executed in the container's main process, which runs in the foreground and blocks the // container from exiting until it finishes. // It waits until the script that is running the tests (see below execCreateCmdResponse) is completed, and until the result files are extracted which is indicated // by the creation of a file "stop_container.txt" in the container's root directory. - .withCmd("sh", "-c", "while [ ! -f " + LOCALCI_WORKING_DIRECTORY + "/stop_container.txt ]; do sleep 0.5; done"); - // .withCmd("tail", "-f", "/dev/null"); // Activate for debugging purposes instead of the above command to get a running container that you can peek into using - // "docker exec -it /bin/bash". - - if (exerciseEnvVars != null && !exerciseEnvVars.isEmpty()) { - envVars.addAll(exerciseEnvVars); - createContainerCmd.withEnv(envVars); - } - - return createContainerCmd.exec(); + .withCmd("sh", "-c", "while [ ! -f " + LOCALCI_WORKING_DIRECTORY + "/stop_container.txt ]; do sleep 0.5; done") + // .withCmd("tail", "-f", "/dev/null") // Activate for debugging purposes instead of the above command to get a running container that you can peek into using + // "docker exec -it /bin/bash". + .exec(); } /** @@ -140,7 +136,12 @@ public void runScriptInContainer(String containerId, String buildJobId, boolean // The "sh preScript.sh" execution command specified here is need to install dependencies and prepare the environment for the build script. log.info("Started running the pre-script for build job in container with id {}", containerId); executeDockerCommand(containerId, buildJobId, true, true, false, "bash", LOCALCI_WORKING_DIRECTORY + "/preScript.sh"); - dockerClient.disconnectFromNetworkCmd().withContainerId(containerId).withNetworkId("bridge").exec(); + try { + dockerClient.disconnectFromNetworkCmd().withContainerId(containerId).withNetworkId("bridge").exec(); + } + catch (Exception e) { + log.error("Failed to disconnect container with id {} from network: {}", containerId, e.getMessage()); + } } log.info("Started running the build script for build job in container with id {}", containerId); diff --git a/src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildJobExecutionService.java b/src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildJobExecutionService.java index da749243d355..5fd47b3eb69a 100644 --- a/src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildJobExecutionService.java +++ b/src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildJobExecutionService.java @@ -192,7 +192,7 @@ public BuildResult runBuildJob(BuildJobQueueItem buildJob, String containerName) index++; } - List envVars = new ArrayList<>(); + List envVars = null; boolean isNetworkDisabled = false; if (buildJob.buildConfig().dockerRunConfig() != null) { envVars = buildJob.buildConfig().dockerRunConfig().getEnv(); diff --git a/src/main/java/de/tum/cit/aet/artemis/programming/domain/ProgrammingExerciseBuildConfig.java b/src/main/java/de/tum/cit/aet/artemis/programming/domain/ProgrammingExerciseBuildConfig.java index a60ca4c8b497..e8c4ae9b4a32 100644 --- a/src/main/java/de/tum/cit/aet/artemis/programming/domain/ProgrammingExerciseBuildConfig.java +++ b/src/main/java/de/tum/cit/aet/artemis/programming/domain/ProgrammingExerciseBuildConfig.java @@ -330,12 +330,12 @@ public DockerRunConfig getDockerRunConfig() { for (List entry : list) { if (entry.size() != 2 || entry.get(0) == null || entry.get(1) == null || entry.get(0).isBlank() || entry.get(1).isBlank() || !DockerRunConfig.AllowedDockerFlags.isAllowed(entry.get(0))) { - log.error("Invalid Docker flag entry: {}. Skipping.", entry); + log.warn("Invalid Docker flag entry: {}. Skipping.", entry); continue; } switch (entry.get(0)) { case "network": - dockerRunConfig.setNetworkDisabled(entry.get(1).equals("none")); + dockerRunConfig.setNetworkDisabled(entry.get(1).equalsIgnoreCase("none")); break; case "env": dockerRunConfig.setEnv(parseEnvVariableString(entry.get(1))); diff --git a/src/main/java/de/tum/cit/aet/artemis/programming/service/ci/ContinuousIntegrationService.java b/src/main/java/de/tum/cit/aet/artemis/programming/service/ci/ContinuousIntegrationService.java index edde994fba48..1e73fb8dfe38 100644 --- a/src/main/java/de/tum/cit/aet/artemis/programming/service/ci/ContinuousIntegrationService.java +++ b/src/main/java/de/tum/cit/aet/artemis/programming/service/ci/ContinuousIntegrationService.java @@ -327,6 +327,7 @@ private static String getBaseScript() { cd /var/tmp/testing-dir #!/usr/bin/env bash set -e + echo "Starting dependency download script" """; } diff --git a/src/main/webapp/app/exercises/programming/manage/update/update-components/custom-build-plans/programming-exercise-build-configuration/programming-exercise-build-configuration.component.ts b/src/main/webapp/app/exercises/programming/manage/update/update-components/custom-build-plans/programming-exercise-build-configuration/programming-exercise-build-configuration.component.ts index 7ae8be6be214..ee9ea1950cb2 100644 --- a/src/main/webapp/app/exercises/programming/manage/update/update-components/custom-build-plans/programming-exercise-build-configuration/programming-exercise-build-configuration.component.ts +++ b/src/main/webapp/app/exercises/programming/manage/update/update-components/custom-build-plans/programming-exercise-build-configuration/programming-exercise-build-configuration.component.ts @@ -41,7 +41,7 @@ export class ProgrammingExerciseBuildConfigurationComponent implements OnInit { ngOnInit() { if (this.programmingExercise()?.buildConfig?.dockerFlags) { - const dockerFlags = JSON.parse(this.programmingExercise()?.buildConfig?.dockerFlags || '{}') as [string, string][]; + const dockerFlags = JSON.parse(this.programmingExercise()?.buildConfig?.dockerFlags || '[]') as [string, string][]; dockerFlags.forEach(([key, value]) => { if (key === this.NETWORK_KEY) { this.isNetworkDisabled = value === 'none'; @@ -64,9 +64,6 @@ export class ProgrammingExerciseBuildConfigurationComponent implements OnInit { updateDockerFlags(key: string, value: string) { let existingFlags = JSON.parse(this.programmingExercise()?.buildConfig?.dockerFlags || '[]') as [string, string][]; - if (!existingFlags || existingFlags.length === 0) { - existingFlags = [] as [string, string][]; - } existingFlags = existingFlags.filter(([flag]) => ALLOWED_DOCKER_FLAG_OPTIONS.includes(flag) && flag !== key) || []; if (ALLOWED_DOCKER_FLAG_OPTIONS.includes(key) && value.trim() !== '') { existingFlags.push([key, value]); diff --git a/src/test/java/de/tum/cit/aet/artemis/programming/icl/LocalVCLocalCIIntegrationTest.java b/src/test/java/de/tum/cit/aet/artemis/programming/icl/LocalVCLocalCIIntegrationTest.java index f209f58ba437..7e8e2954313d 100644 --- a/src/test/java/de/tum/cit/aet/artemis/programming/icl/LocalVCLocalCIIntegrationTest.java +++ b/src/test/java/de/tum/cit/aet/artemis/programming/icl/LocalVCLocalCIIntegrationTest.java @@ -972,7 +972,7 @@ void testFetchPush_instructorPracticeRepository() throws Exception { } @Nested - class DisabledBuildAgentTest { + class BuildJobConfigurationTest { @BeforeEach void setUp() {