-
Notifications
You must be signed in to change notification settings - Fork 2.9k
NIFI-15148 - Registry Clients - Support for branch creation #10471
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
base: main
Are you sure you want to change the base?
Conversation
dce8ef2 to
c3d55fc
Compare
exceptionfactory
left a comment
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.
Thanks for implementing this new feature @pvillard31.
I reviewed the service changes and the majority of the implementation looks good. I noted a few minor recommendations.
...ifi-web/nifi-web-api/src/main/java/org/apache/nifi/web/dao/impl/StandardFlowRegistryDAO.java
Outdated
Show resolved
Hide resolved
...ifi-web/nifi-web-api/src/main/java/org/apache/nifi/web/dao/impl/StandardFlowRegistryDAO.java
Outdated
Show resolved
Hide resolved
...ework/nifi-web/nifi-web-api/src/main/java/org/apache/nifi/web/StandardNiFiServiceFacade.java
Outdated
Show resolved
Hide resolved
...ework/nifi-web/nifi-web-api/src/main/java/org/apache/nifi/web/StandardNiFiServiceFacade.java
Outdated
Show resolved
Hide resolved
| .thenReturn(expectedEntity); | ||
|
|
||
| final Response response = versionsResource.createFlowBranch(groupId, requestEntity); | ||
| assertEquals(200, response.getStatus()); |
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.
HttpURLConnection.HTTP_OK can be used in place for 200 here and in other methods.
| final NiFiCoreException exception = assertThrows(NiFiCoreException.class, | ||
| () -> dao.createBranchForUser(userContext, "registry-1", sourceLocation, "feature")); | ||
|
|
||
| assertEquals("Unable to create branch [feature] in registry with ID registry-1: Branch [feature] already exists", exception.getMessage()); |
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.
In general it is best to avoid asserting an exact message, recommend asserting that the message contains a particular keyword, such as registry-1 if that is the goal.
| final NiFiCoreException exception = assertThrows(NiFiCoreException.class, | ||
| () -> serviceFacade.createFlowBranch(revision, PROCESS_GROUP_ID, "feature", null, null)); | ||
|
|
||
| assertEquals("Unable to create branch [feature] in registry with ID registry-1: Branch [feature] already exists", exception.getMessage()); |
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.
See note on message matching.
|
Thanks @exceptionfactory - I pushed a commit to address your comments |
Signed-off-by: Pierre Villard <[email protected]>
Co-authored-by: David Handermann <[email protected]>
c8fbd26 to
0e4e7f6
Compare
Summary
NIFI-15148 - Registry Clients - Support for branch creation
It requires apache/nifi-api#25 and a new NiFi API release.
Not very satisfied with the "Create Branch" modal view but I'm sure someone more familiar with the UI will be able to help getting something better.
Tracking
Please complete the following tracking steps prior to pull request creation.
Issue Tracking
Pull Request Tracking
NIFI-00000NIFI-00000Pull Request Formatting
mainbranchVerification
Please indicate the verification steps performed prior to pull request creation.
Build
./mvnw clean install -P contrib-checkLicensing
LICENSEandNOTICEfilesDocumentation