-
Notifications
You must be signed in to change notification settings - Fork 4
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
add backend checks for BuildInfo #58
base: main
Are you sure you want to change the base?
Conversation
e22ec67
to
4e18d52
Compare
private static final String VALID_REPO = "https://github.com/test/repo.git"; | ||
private static final String VALID_VERSION = "1.0.0"; | ||
private static final String VALID_COMMIT_SHA = "61ec86095de795f5fb817a7cc824d8d7cfb9ae51"; | ||
private static final List<String> VALID_STREAMS = List.of("EAP_7_3_X"); |
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 should probably be replaced with List.of(Stream.EAP_7_3_X.frontEnd)
|
||
// Test field annotations validation | ||
|
||
private final Validator validator; |
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'd prefere non-test methods (and classes) to be after the tests
4e18d52
to
70b2d26
Compare
|
||
import java.util.List; | ||
|
||
@JsonDeserialize(builder = BuildInfo.Builder.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.
Why do we need to use a builder? Constraints like NotBlank, Pattern, and so on should be enough or not?
Trimming can be simply done through a set method (it is generated by Quarkus but we might override it simply by adding it here if needed) e.g.:
public void setTag(@NotNull String tag) {
this.tag = tag.trim();
}
Is there any other reason I am missing here?
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 decided to use Builder and not setters because I think once BuildInfo is created, it is not supposed to be changed or edited. Or why do you think setters would be better?
Field annotations are only validated when we pass the BuildInfo through the @Valid annotation, which does not happen in GitBuildInfoAssembler, for example.
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.
You are right, I forgot about creation in GitBuildInfoAssembler for a sec.
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 am still unsure about this, because it looks a bit messy for such a small project. Isn't there a way how we can at least do the validation for both cases in one place? I am unsure because we are creating BuildInfo in 2 places (if I omit test cases) and we are using 2 different approaches for that. I think it is unnecessary complex to create and use Builder only for one specific usage. But I am not sure how to avoid that for now, so I guess I am okay with Builder now.
|
||
public List<String> streams; | ||
// Maximum of 5 streams allowed, based on Stream.values().length. | ||
@Size(max = 5, message = "The maximum number of streams allowed is 5.") |
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.
Minimal size = 1 should be also defined
public String tag; | ||
@NotBlank | ||
@Pattern(regexp = "^https://.+$", message = "Tag must start with https://") | ||
private final String tag; |
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.
Also, please remove public access modifiers and leave it as package-private if no specific reason to do it otherwise. These were left there by mistake.
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 am not sure what you mean by this, I replaced public with private modifiers.
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.
That was for case we would not use Builder, so you can ignore it. :)
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.
For now, I would say only the minimal size of streams needs to be taken care of. In UI we can't create a Build request if at least one stream is not chosen.
|
||
import java.util.List; | ||
|
||
@JsonDeserialize(builder = BuildInfo.Builder.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.
I am still unsure about this, because it looks a bit messy for such a small project. Isn't there a way how we can at least do the validation for both cases in one place? I am unsure because we are creating BuildInfo in 2 places (if I omit test cases) and we are using 2 different approaches for that. I think it is unnecessary complex to create and use Builder only for one specific usage. But I am not sure how to avoid that for now, so I guess I am okay with Builder now.
resolves #56