Gitlab container and tests #1335

ampuscas · 2023-08-16T12:30:05Z

Testing done

Submitter checklist

Give feedback

Make sure you are opening from a **topic/feature/bugfix branch** (right side) and not your main branch!

Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
Options
Successfully updated the issue's project

There was an error updating the issue's project
Ensure that the pull request title represents the desired changelog entry

Ensure that the pull request title represents the desired changelog entry
Options
Successfully updated the issue's project

There was an error updating the issue's project
Please describe what you did

Please describe what you did
Options
Successfully updated the issue's project

There was an error updating the issue's project
Link to relevant issues in GitHub or Jira

Link to relevant issues in GitHub or Jira
Options
Successfully updated the issue's project

There was an error updating the issue's project
Link to relevant pull requests, esp. upstream and downstream changes

Link to relevant pull requests, esp. upstream and downstream changes
Options
Successfully updated the issue's project

There was an error updating the issue's project
Ensure you have provided tests - that demonstrates feature works or fixes the issue

Ensure you have provided tests - that demonstrates feature works or fixes the issue
Options
Successfully updated the issue's project

There was an error updating the issue's project
Options

src/main/resources/org/jenkinsci/test/acceptance/docker/fixtures/GitLabContainer/Dockerfile

timja · 2023-08-16T15:01:06Z

pom.xml

+    <dependency>
+      <groupId>com.squareup.okhttp3</groupId>
+      <artifactId>okhttp</artifactId>
+      <version>5.0.0-alpha.11</version>


is there a reason you aren't using the built-in Java HttpClient?
https://www.baeldung.com/java-9-http-client

this avoids adding another dependency, either way probably shouldn't be using an alpha one

I switched this over to using java HttpClient. There seems to be lots of configuration options for the java http client, please let me know if you think there is one we should have that I am missing.

I really appreciate your feedback and support with this @timja 😃

…t the methods

raul-arabaolaza

Leave several comments

raul-arabaolaza · 2023-08-21T08:22:40Z

src/main/java/org/jenkinsci/test/acceptance/docker/fixtures/GitLabContainer.java

+        Response response = null;
+        try {
+            response = client.newCall(request).execute();
+        } catch (IOException e) {


Why this mapping?

What do you mean by this?

Comment resolved due to switching over to java http client

raul-arabaolaza · 2023-08-21T08:23:41Z

src/main/java/org/jenkinsci/test/acceptance/docker/fixtures/GitLabContainer.java

+        return response;
+    }
+
+    public void deleteRepo(String token) throws IOException {


Suggested change

public void deleteRepo(String token) throws IOException {

public void deleteRepo(String repo) throws IOException {

You do not need the token as there is a getter for it, but you should not assume there is only one repo...

raul-arabaolaza · 2023-08-21T08:24:13Z

src/main/java/org/jenkinsci/test/acceptance/docker/fixtures/GitLabContainer.java

+        ProjectApi projApi = new ProjectApi(gitlabapi);
+
+        try {
+            Project project = projApi.getProjects().get(0);


What if more than one test is running in parallel? There will be more than one repo

raul-arabaolaza · 2023-08-21T08:24:47Z

src/main/java/org/jenkinsci/test/acceptance/docker/fixtures/GitLabContainer.java

+        }
+    }
+
+    public String createUserToken() throws IOException, InterruptedException {


Suggested change

public String createUserToken() throws IOException, InterruptedException {

public String createUser(String username) throws IOException, InterruptedException {

This should allow you to create any user you want and return the token for further auth if needed

raul-arabaolaza · 2023-08-21T08:35:31Z

src/main/resources/org/jenkinsci/test/acceptance/docker/fixtures/GitLabContainer/create_user.rb

@@ -0,0 +1,13 @@
+user = User.create();
+user.name = 'test';


Hardcoded, what if this is called by tests in parallel? Is gonna fail. Maybe you can try what is described here https://www.codecademy.com/article/ruby-command-line-argv

what if this is called by tests in parallel

then each test should have its own instance of the container so it doesn't matter (unless we are running multiple tests in the same JVM at once which we are not (threading as opposed to forking))

raul-arabaolaza · 2023-08-21T08:35:54Z

src/main/resources/org/jenkinsci/test/acceptance/docker/fixtures/GitLabContainer/create_user.rb

+user = User.create();
+user.name = 'test';
+user.username = 'test';
+user.password = 'oparolaoarecare12!';


Same as before, this should be an argument comming from the java call

raul-arabaolaza · 2023-08-21T08:36:07Z

src/main/resources/org/jenkinsci/test/acceptance/docker/fixtures/GitLabContainer/create_user.rb

+user.password = 'oparolaoarecare12!';
+user.confirmed_at = '01/01/1990';
+user.admin = true;
+user.email = '$test@example.com';


.github/renovate.json

…ance-test-harness into gitlab-container

julieheard · 2023-08-21T13:30:37Z

pom.xml

+      <groupId>com.squareup.okhttp3</groupId>
+      <artifactId>okhttp</artifactId>
+      <version>5.0.0-alpha.11</version>
+    </dependency>


Ignore that this is in here twice - we have Andra and I working on the same PR so just caught in the crossfire. We are removing as moving to http client shortly so ignore for now 🙂

raul-arabaolaza · 2023-08-21T14:50:21Z

src/main/java/org/jenkinsci/test/acceptance/docker/fixtures/GitLabContainer.java

+        try {
+            Project project = projApi.getProjects().get(0);
+            projApi.deleteProject(project.getId());
+        } catch (GitLabApiException e) {


No need to wrap inside a RuntimeException just throw it and the test will fail if thrown

src/main/java/org/jenkinsci/test/acceptance/docker/fixtures/GitLabContainer.java

src/test/java/plugins/GitLabPluginTest.java

jtnord

Should this be in draft until there is a test that exercises some functionality in Jenkins provided by the GitLab plugin?

jtnord · 2023-08-22T10:07:21Z

src/main/java/org/jenkinsci/test/acceptance/docker/fixtures/GitLabContainer.java

+    public HttpResponse<String> createRepo(String repoName, String token) throws IOException {
+        try{
+            HttpRequest request = HttpRequest.newBuilder()
+                                             .uri(new URI("http://" + getIpAddress() + "/api/v4/projects"))


I think this will not work in all scenarious - the web port has not been configured as exposed (@DockerFixture(id = "gitlab-plugin", ports = 22)) and as such you may not be able to communicate witht the web interface of GitLab depending on your docker setup. IIUC you should add a new mapping for the web port.

Additionally you have getURL() that you should call to then addon the endpoint rather than repeating yourself.

src/main/java/org/jenkinsci/test/acceptance/docker/fixtures/GitLabContainer.java

jtnord · 2023-08-22T10:09:15Z

src/main/java/org/jenkinsci/test/acceptance/docker/fixtures/GitLabContainer.java

+
+    public URL getURL() throws IOException {
+        // return new URL("http://" + host() + ":" + port());
+        return new URL("http://" + getIpAddress());


you need to use the ipaddress and port that is mapped here.

src/main/java/org/jenkinsci/test/acceptance/docker/fixtures/GitLabContainer.java

jtnord · 2023-08-22T10:11:53Z

src/main/resources/org/jenkinsci/test/acceptance/docker/fixtures/GitLabContainer/create_user.rb

@@ -0,0 +1,13 @@
+user = User.create();
+user.name = 'test';


what if this is called by tests in parallel

then each test should have its own instance of the container so it doesn't matter (unless we are running multiple tests in the same JVM at once which we are not (threading as opposed to forking))

jtnord · 2023-08-22T10:13:58Z

src/main/java/org/jenkinsci/test/acceptance/docker/fixtures/GitLabContainer.java

+                .until( () ->  {
+                    try {
+                          HttpRequest request = HttpRequest.newBuilder()
+                                .uri(new URI("http://" + getIpAddress()))


use getURL()

jtnord · 2023-08-22T10:16:20Z

src/main/java/org/jenkinsci/test/acceptance/docker/fixtures/GitLabContainer.java

+    public void waitForReady(CapybaraPortingLayer p) {
+        int timeout = (int) time.milliseconds(200000);
+        p.waitFor().withMessage("Waiting for GitLab to come up")
+                .withTimeout(Duration.ofSeconds(timeout)) // GitLab starts in about 2 minutes


would be so much more readable with something like

Suggested change

.withTimeout(Duration.ofSeconds(timeout)) // GitLab starts in about 2 minutes

.withTimeout(Duration.ofSeconds(60 * 2)) // GitLab starts in about 2 minutes

or
suggestion
.withTimeout(Duration.ofMinutes(2)) // GitLab starts in about 2 minutes

jtnord

some incorect use of Elastic time with wait.

src/main/java/org/jenkinsci/test/acceptance/plugins/gitlab_plugin/GitLabServerConfig.java

src/test/java/plugins/GitLabPluginTest.java

src/main/java/org/jenkinsci/test/acceptance/docker/fixtures/GitLabContainer.java

src/main/java/org/jenkinsci/test/acceptance/plugins/gitlab_plugin/GitLabOrganizationFolder.java

src/test/java/plugins/GitLabPluginTest.java

rsandell · 2023-09-14T10:34:02Z

src/main/resources/org/jenkinsci/test/acceptance/docker/fixtures/GitLabContainer/create_user.rb

+user.save!;
+
+token = user.personal_access_tokens.create(scopes: [:api], name: 'MyToken');
+token.expires_at='01/01/2024';


This will be problematic in 4 months.

rsandell · 2023-09-14T10:40:42Z

src/test/java/plugins/GitLabPluginTest.java

+        this.assertExistAndResult(failureJob, false);
+
+        // delete the repo when finished
+        container.deleteRepo(getPrivateTokenAdmin(), repoName);


This should be done in a @After method or in a try { ... } finally {...} block because if any of the asserts above fails then the repo won't be deleted otherwise.

rsandell · 2023-09-14T10:45:39Z

src/test/java/plugins/GitLabPluginTest.java

+
+    private String groupName = "firstgroup";
+
+    private String JenkinsfileContent = "pipeline {\n" +


This pipeline is using declarative syntax which should mean you need to declare the pipeline-model-definition plugin in the @WithPlugins annotation.

raul-arabaolaza · 2023-09-15T15:20:06Z

Can we get this merged, please? No blockers apparently and the builder is happy

jtnord · 2023-09-18T11:41:42Z

@rsandell master is broken - looks like this PR is broken on java21?

timja · 2023-09-20T09:17:23Z

Can this please be looked at quickly?

It's breaking all PRs.

car-roll · 2023-09-20T16:47:48Z

This is from my first look at these failures: #1364 (comment)

car-roll · 2023-09-21T03:26:18Z

Possible fix #1365

ampuscas added 2 commits August 11, 2023 17:33

add docker fixture for gitlab

3afb2d0

create test

cca1c47

timja reviewed Aug 16, 2023

View reviewed changes

src/main/resources/org/jenkinsci/test/acceptance/docker/fixtures/GitLabContainer/Dockerfile Outdated Show resolved Hide resolved

julieheard added 4 commits August 16, 2023 15:05

pin gitlab version

fc26010

Remove addSSHCertificate in GitLabContainer

4737ba8

add createRepo test

351b43f

Reword createRepo and add TODO

4d3bb15

timja reviewed Aug 16, 2023

View reviewed changes

ampuscas added 2 commits August 17, 2023 11:17

add delete repo method

c848476

automatically create a token to be used inside the container and adap…

16db779

…t the methods

raul-arabaolaza reviewed Aug 21, 2023

View reviewed changes

add gitlab to renovate

fd1df51

timja reviewed Aug 21, 2023

View reviewed changes

.github/renovate.json Outdated Show resolved Hide resolved

ampuscas added 2 commits August 21, 2023 15:22

wait for gitlab to be running

14f2fef

Merge branch 'gitlab-container' of https://github.com/ampuscas/accept…

199851b

…ance-test-harness into gitlab-container

julieheard reviewed Aug 21, 2023

View reviewed changes

julieheard and others added 2 commits August 21, 2023 14:57

Changing over to java http client and cleanup

932b1ba

modify the wait method

5b13cb8

raul-arabaolaza reviewed Aug 21, 2023

View reviewed changes

src/main/java/org/jenkinsci/test/acceptance/docker/fixtures/GitLabContainer.java Outdated Show resolved Hide resolved

raul-arabaolaza reviewed Aug 21, 2023

View reviewed changes

src/main/java/org/jenkinsci/test/acceptance/docker/fixtures/GitLabContainer.java Outdated Show resolved Hide resolved

raul-arabaolaza reviewed Aug 21, 2023

View reviewed changes

src/test/java/plugins/GitLabPluginTest.java Outdated Show resolved Hide resolved

use repo name in the delete method

d1f437a

ampuscas marked this pull request as ready for review August 22, 2023 09:18

code review

145e570

jtnord reviewed Aug 22, 2023

View reviewed changes

ampuscas added 4 commits August 22, 2023 16:11

create multiple users

81fc055

configure dependabot for gitlab image

7f25a0c

try port mapping

ec4451f

fix waitForReady

fab75d9

ampuscas added 4 commits September 8, 2023 09:05

code review part 1

5bf9094

code review part 2

b3b0c02

code review part 3

6cc5fc9

code review part 4

2450366

ampuscas requested review from rsandell, jtnord, timja, raul-arabaolaza, car-roll and julieheard September 11, 2023 15:23

jtnord reviewed Sep 11, 2023

View reviewed changes

jtnord reviewed Sep 12, 2023

View reviewed changes

src/test/java/plugins/GitLabPluginTest.java Outdated Show resolved Hide resolved

refactor and code review

51a34da

ampuscas requested a review from jtnord September 12, 2023 13:54

car-roll approved these changes Sep 13, 2023

View reviewed changes

rsandell reviewed Sep 14, 2023

View reviewed changes

rsandell approved these changes Sep 14, 2023

View reviewed changes

raul-arabaolaza approved these changes Sep 14, 2023

View reviewed changes

raul-arabaolaza approved these changes Sep 15, 2023

View reviewed changes

rsandell added enhancement tests labels Sep 18, 2023

rsandell changed the title ~~Gitlab container~~ Gitlab container and tests Sep 18, 2023

rsandell merged commit 5e76540 into jenkinsci:master Sep 18, 2023
23 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Gitlab container and tests #1335

Gitlab container and tests #1335

ampuscas commented Aug 16, 2023

Submitter checklist

timja Aug 16, 2023

julieheard Aug 21, 2023

raul-arabaolaza left a comment

raul-arabaolaza Aug 21, 2023

julieheard Aug 21, 2023

julieheard Aug 21, 2023

raul-arabaolaza Aug 21, 2023

raul-arabaolaza Aug 21, 2023

raul-arabaolaza Aug 21, 2023

raul-arabaolaza Aug 21, 2023

raul-arabaolaza Aug 21, 2023

jtnord Aug 22, 2023

raul-arabaolaza Aug 21, 2023

raul-arabaolaza Aug 21, 2023

julieheard Aug 21, 2023 •

edited by batmat

raul-arabaolaza Aug 21, 2023

jtnord left a comment

jtnord Aug 22, 2023

jtnord Aug 22, 2023

jtnord Aug 22, 2023

jtnord Aug 22, 2023

jtnord Aug 22, 2023

jtnord left a comment

rsandell Sep 14, 2023

rsandell Sep 14, 2023

rsandell Sep 14, 2023

raul-arabaolaza commented Sep 15, 2023

jtnord commented Sep 18, 2023

timja commented Sep 20, 2023

car-roll commented Sep 20, 2023

car-roll commented Sep 21, 2023

	public void deleteRepo(String token) throws IOException {
	public void deleteRepo(String repo) throws IOException {

	public String createUserToken() throws IOException, InterruptedException {
	public String createUser(String username) throws IOException, InterruptedException {

	.withTimeout(Duration.ofSeconds(timeout)) // GitLab starts in about 2 minutes
	.withTimeout(Duration.ofSeconds(60 * 2)) // GitLab starts in about 2 minutes


		private String groupName = "firstgroup";

		private String JenkinsfileContent = "pipeline {\n" +

Gitlab container and tests #1335

Gitlab container and tests #1335

Conversation

ampuscas commented Aug 16, 2023

Testing done

Submitter checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raul-arabaolaza left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julieheard Aug 21, 2023 • edited by batmat

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtnord left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtnord left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raul-arabaolaza commented Sep 15, 2023

jtnord commented Sep 18, 2023

timja commented Sep 20, 2023

car-roll commented Sep 20, 2023

car-roll commented Sep 21, 2023

julieheard Aug 21, 2023 •

edited by batmat