-
Notifications
You must be signed in to change notification settings - Fork 2.9k
NIFI-14968 - Add support for Bitbucket Data Center #10322
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
be14768 to
5f44977
Compare
5f44977 to
8bcc4bf
Compare
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 the substantial work on implementing support for Bitbucket Data Center @pvillard31. Most of the changes look clear enough, I noted a few recommendations.
...-client-api/src/main/java/org/apache/nifi/web/client/api/MultipartFormDataStreamBuilder.java
Show resolved
Hide resolved
| final String boundaryPrefix = (index == 0 ? BOUNDARY_SEPARATOR + boundary + CARRIAGE_RETURN_LINE_FEED | ||
| : CARRIAGE_RETURN_LINE_FEED + BOUNDARY_SEPARATOR + boundary + CARRIAGE_RETURN_LINE_FEED); |
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 ternary and string concatenation is rather difficult to read. Having something like the getFooter method would be helpful.
| public enum BitbucketAuthenticationType implements DescribedValue { | ||
|
|
||
| BASIC_AUTH("Basic Auth", """ | ||
| BASIC_AUTH("Basic Auth & API Token", """ |
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.
Recommend avoiding the use of the ampersand character in labels. On the other hand, is it necessary to mention API Token in the name when it is in the description?
| BASIC_AUTH("Basic Auth & API Token", """ | |
| BASIC_AUTH("Basic Auth and API Token", """ |
| static final PropertyDescriptor APP_PASSWORD = new PropertyDescriptor.Builder() | ||
| .name("App Password") | ||
| .description("The App Password to use for authentication") | ||
| .displayName("App Password or API Token") |
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.
Instead of adding the displayName, it would be better to migrate the property name. Alternatively, for the sake of clarity, it might be better to have a separate property.
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.
migrateProperties is not available for registry clients, looking into having a separate property
| final String apiHost = bitbucketRepositoryClient.getApiHost(); | ||
| final String projectKey = bitbucketRepositoryClient.getProjectKey(); | ||
| if (apiHost != null && projectKey != null) { | ||
| return "git@" + apiHost + ":" + projectKey + "/" + bitbucketRepositoryClient.getRepoName() + ".git"; |
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.
Recommend using string formatting instead of concatenation to make this structure easier to follow.
| .header(CONTENT_TYPE_HEADER, multipartBuilder.getHttpContentType().getContentType()) | ||
| .retrieve(); | ||
|
|
||
| if (response.statusCode() != HttpURLConnection.HTTP_OK && response.statusCode() != HttpURLConnection.HTTP_CREATED) { |
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.
Is it possible to return either 200 or 201? It seems like it should be one or the other.
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.
Yes, it can be one or the other depending on potential existing content. I updated the logic to make it easier to understand.
| throw new FlowRegistryException(String.format("Could not parse Bitbucket API response at %s", uri), e); | ||
| } | ||
|
|
||
| final JsonNode values = root.path("values"); |
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.
It looks like there are a number of repeated uses of certain expected field names, which could be useful to declare as static final variables.
|
Thanks for the review @exceptionfactory - pushed a commit addressing your comments |
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 the updates @pvillard31. I noted what appear to be a few opportunities to define reusable HTTP request and JSON response parsing, otherwise this looks close to completion.
| if (formFactor == BitbucketFormFactor.DATA_CENTER) { | ||
| return commit.path("id").asText(""); | ||
| } | ||
| return commit.path("hash").asText(""); |
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.
It looks like "" could be declared as EMPTY_STRING and reused.
| throw new FlowRegistryException("Could not parse response from Bitbucket API", e); | ||
| } | ||
|
|
||
| final JsonNode values = jsonResponse.get("values"); |
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.
It looks like values is used in a number of methods.
| } | ||
|
|
||
| private boolean isFileEntry(final JsonNode entry) { | ||
| final JsonNode typeNode = entry.get("type"); |
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.
type and path appear to be used in a number of places.
| if (formFactor == BitbucketFormFactor.DATA_CENTER) { | ||
| return "FILE".equalsIgnoreCase(type); | ||
| } | ||
| return "commit_file".equals(type); |
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 FILE be upper or lower case?
| final JsonNode root; | ||
| try { | ||
| root = objectMapper.readTree(response.body()); | ||
| } catch (IOException e) { | ||
| throw new FlowRegistryException(String.format("Could not parse Bitbucket API response at %s", uri), e); | ||
| } |
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 seems to be a common pattern, it would be helpful to create a shared method and reuse instead of copy the try-catch block across methods.
| final HttpResponseEntity response = this.webClient.getWebClientService() | ||
| .get() | ||
| .uri(uri) | ||
| .header(AUTHORIZATION_HEADER, authToken.getAuthzHeaderValue()) | ||
| .retrieve(); | ||
|
|
||
| if (response.statusCode() != HttpURLConnection.HTTP_OK) { | ||
| final String errorMessage = String.format("Error while listing commits for repository [%s] on branch %s", repoName, branch); | ||
| throw new FlowRegistryException(errorMessage + ": " + getErrorMessage(response)); | ||
| } |
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.
It seems like there is an opportunity to have a reusable method that checks for the expected status code.
| final List<InputStream> streams = new ArrayList<>(); | ||
| for (int index = 0; index < parts.size(); index++) { | ||
| final Part part = parts.get(index); | ||
| streams.add(new ByteArrayInputStream(getBoundaryPrefix(index).getBytes(HEADERS_CHARACTER_SET))); |
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.
Recommend declaring the return of getBoundaryPrefix as a separate variable for readability instead of the nested call and conversion to a byte array.
|
Thanks @exceptionfactory - pushed another commit |
Summary
NIFI-14968 - Add support for Bitbucket Data Center
Breaking Change - the API version property is removed and Registry Clients do not support the migrateProperties feature so there is no nice way to remove it. However I think this is OK and can be clearly identified in the release notes / migration notes. Happy to re-add the property if needed and just ignore it.
Important - for Data Center, when committing a file using multipart, the existing code was not working also sending data using curl was working as expected. It turned out that the multipart was not formatted properly and it was expecting the filename as part of the content disposition.
Also... as of September 9th, Bitbucket does not allow users to create App Passwords anymore and API Tokens are the long term replacement. I have made the changes to reflect this.
Most of the work is to decouple the Data Center 1.0 APIs and the Cloud 2.0 APIs. I also fixed an issue that existed in case the registry client is used against a repo that has just been created with no commit at all (quite rare but this is now supported).
The change has been tested against both a Cloud and a Data Center instance. For Data Center, I tested against the latest version Data Center 10.0.
All the usual scenarios have been tested: branching, change version, commit version, start version control, as well as some of the usual edge cases.
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