-
Notifications
You must be signed in to change notification settings - Fork 467
Added missing attributes for ProjectApi #1237
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
Conversation
@@ -789,7 +828,7 @@ public void setBuildCoverageRegex(String buildCoverageRegex) { | |||
|
|||
public Project withBuildCoverageRegex(String buildCoverageRegex) { | |||
this.buildCoverageRegex = buildCoverageRegex; | |||
return this; | |||
return (this); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see why we need those brackets.
I would just do this:
return (this); | |
return this; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like a very good addition.
The example file project.json
used in unit-test should be updated to match the changes made to the model class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you update the file project.json
?
https://github.com/gitlab4j/gitlab4j-api/blob/fe14e34f0ab11e9f06f52765f86fbc0d72caa3dd/gitlab4j-models/src/test/resources/org/gitlab4j/models/project.json
Ideally with values coming from an existing from a real REST call.
This way we would we would ensure that your changes are correctly serialized and deserialized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some properties are only configurable with a Premium/Ultimate gitlab server, which I unfortenately do not have access to, so I've used an accepted value based on the documentation:
option | type | value I used |
---|---|---|
use_custom_template | boolean | false |
external_authorization_classification_label | string | "label" |
mirror_trigger_builds | boolean | false |
only_allow_merge_if_all_status_checks_passed | boolean | false |
group_with_project_templates_id | integer | 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some properties are only configurable with a Premium/Ultimate gitlab server, which I unfortenately do not have access to
Yes this is a pity… even for working on gitlab4j there is no good way to access such a license (or I didn't asked at the correct place)
"avatar" : null, | ||
"group_with_project_templates_id" : null, | ||
"public_builds" : null, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using null
in those test file does not work with the unit test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, I missed those. I have also removed the avatar
property, since that can only be used with a multipart/form-data and I now saw that there already is a setProjectAvatar
method that uses this property
public class ContainerExpirationPolicy implements Serializable { | ||
private static final long serialVersionUID = 1L; | ||
|
||
private String cadence; | ||
private Boolean enabled; | ||
|
||
@JsonProperty("keep_n") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently the way the models are written, we are not using @JsonProperty
. There was an attempt in #1198, but it never was finished.
For consistency reason, I would not add this now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had to use @JsonProperty
, so that the snake_case variant is used, instead of the camelCase.
With @JsonProperty
:
{
"cadence" : "1month",
"enabled" : true,
"keep_n" : 100,
"older_than" : "7d",
"name_regex" : ".*-development",
"name_regex_keep" : ".*-main"
}
Without @JsonProperty
:
{
"cadence" : "1month",
"enabled" : true,
"keepN" : 100,
"olderThan" : "7d",
"nameRegex" : ".*-development",
"nameRegexKeep" : ".*-main"
}
Additionally, I also saw it used in Project
itself:
@JsonProperty("_links")
private Map<String, String> links;
* @throws GitLabApiException if any exception occurs | ||
*/ | ||
public Project setProjectContainerExpirationPolicy( | ||
Object projectIdOrPath, ContainerExpirationPolicyAttributes containerExpirationPolicyAttributes) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why aren't you using just ContainerExpirationPolicy
as parameter?
Inside the method, you could create a new Project and pass the parameter to it with withContainerExpirationPolicy(..)
If you serialise that project instance you will get the same result as with containerExpirationPolicyAttributes.toString()
(since all the other fields will be null
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried that before, but it does not work, I keep getting this error: container_expiration_policy_attributes is invalid
According to the documentation, container_expiration_policy_attributes
is used in the create/edit API to modify the policy, while container_expiration_policy
is used in the list API to list the policy.
Also, there is an example which updates the policy, which uses --header 'Content-Type: application/json;charset=UTF-8'
I think it doesn't work with withContainerExpirationPolicy(..)
. since that request eventually uses --header 'Content-Type: application/x-www-form-urlencoded'
. But the container_expiration_policy_attributes
is serialized as JSON:
{
"cadence" : "1month",
"enabled" : true,
"keep_n" : 100,
"older_than" : "7d",
"name_regex" : ".*-development",
"name_regex_keep" : ".*-main"
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The gitlab REST API is very flexible and accept the request to be either a JSON body, a regular HTTP Form Data, query parameters, …
I understand why you need a different method setProjectContainerExpirationPolicy(..)
because here you are sending a project update request where the Project
is serialised as Body.
My point was that ContainerExpirationPolicyAttributes
model is not really necessary.
You could do something like (not tested):
public Project setProjectContainerExpirationPolicy(Object projectIdOrPath, ContainerExpirationPolicy policy) {
Project request = new Project().withContainerExpirationPolicy(policy);
Response response = put(
Response.Status.OK,
request.toString(),
"projects",
getProjectIdOrPath(projectIdOrPath));
return (response.readEntity(Project.class))
}
I don't understand why this would not work.
For me request.toString()
with request
being a Project
instance is the same as your containerExpirationPolicyAttributes.toString()
where your containerExpirationPolicyAttributes
instance is of type ContainerExpirationPolicyAttributes
.
And it removes the need for a ContainerExpirationPolicyAttributes
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, I now see that there is also withParam(String name, Map<String, ?> variables, boolean required)
method in GitLabApiForm
which correctly sets form fields for a Map. I can just use that with container_expiration_policy_attributes
, that also works.
Thank you very much for this contribution 🎉 |
Added missing attributes to the ProjectApi