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

Expose repository_object_format via Project API #1125

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

slonopotamus
Copy link

GitLab 16.9 introduced an option to create repositories with SHA256 instead of SHA1.

This commit exposes object format used by particular repository via Project API.

See https://gitlab.com/gitlab-org/gitlab/-/issues/419887

GitLab 16.9 introduced an option to create repositories with SHA256 instead of SHA1.

This commit exposes object format used by particular repository via Project API.

See https://gitlab.com/gitlab-org/gitlab/-/issues/419887

Signed-off-by: Marat Radchenko <[email protected]>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please also update the json file /gitlab4j-api/src/test/resources/org/gitlab4j/api/project.json that corresponds to this model and that is used in the TestGitLabApiBeans test.

@@ -224,6 +224,7 @@ public void testCreate() throws GitLabApiException {
.withMergeRequestsEnabled(true)
.withWikiEnabled(true)
.withSnippetsEnabled(true)
.withRepositoryObjectFormat("sha1")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I am not mistaken this test is using the GitLab installation started in Docker while the integration tests are running.

This version is very old and this test will fail.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm... Is there any reference test that I could use for this new field?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The pipeline in GitHub action is failing as expected:

Error:  Failures: 
Error:    TestProjectApi.testCreate:239 expected: <sha1> but was: <null>
[INFO] 
Error:  Tests run: 308, Failures: 1, Errors: 0, Skipped: 14

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some tests are using Mockito but this feels also wrong.
I started to experiment to use testcontainers to run a newer GitLab version (there is some work in a PR).

But I am wondering if we should not just Mock the GitLab server in order to run the unit-test quickly.

I would say that for now TestProjectApi.testCreate() remains unchanged and we merge your change.

@slonopotamus
Copy link
Author

@jmini is it ok that repositoryObjectFormat is a string? Or it would be better if it was a enum, like project vsibility?

@jmini
Copy link
Collaborator

jmini commented May 17, 2024

is it ok that repositoryObjectFormat is a string? Or it would be better if it was a enum, like project vsibility?

I am always not really sure about the String vs enum… In the docs they just mention sha1 being the default value but they do not say it is sha1 or sha256. So I think String is OK.

@slonopotamus
Copy link
Author

In the docs they just mention sha1 being the default value but they do not say it is sha1 or sha256

Yes, sha1 is default. The repository can either be created with sha1 or sha256. It cannot be changed after repository ws created. And it is very unlikely any new values will emerge in nearest 10 years or so.

So I think String is OK.

Ok.

@slonopotamus slonopotamus marked this pull request as draft May 17, 2024 12:27
@slonopotamus
Copy link
Author

Converting to draft until I fix test issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants