NIFI-13105 Implement a FlowRegistryClient that use GitHub for version… #8765

bbende · 2024-05-07T17:22:21Z

…ing flows

Summary

This PR adds a new FlowRegistryClient that uses GitHub API to version control flows. Currently the client is configured with the repo information, including a branch and optional path in the repo to use, otherwise the root of the repo is used.

Buckets are directories underneath the location in the repo. The client will check for a bucket named Default, and if it does not exist it will be created. Additional buckets can be created by adding additional directories to the repo outside of NiFi.

The branch is planned to become part of the version control screens when saving/importing which is tracked by. NIFI-13127. Until then, the branch that is configured in the client will be used for all operations, so in order to work on different branches, it currently requires creating separate registry clients pointing at the same repo, but different branches.

It is also possible to import flows from a public repo that you don't have permissions to by not providing an access token. This will allow import and change version, but attempting to save a change will result in a "not found" exception which is GitHub's behavior when you specify a branch or file that you don't have access to, even though it may be in the repo.

Tracking

Please complete the following tracking steps prior to pull request creation.

Issue Tracking

Apache NiFi Jira issue created

Pull Request Tracking

Pull Request title starts with Apache NiFi Jira issue number, such as NIFI-00000
Pull Request commit message starts with Apache NiFi Jira issue number, as such NIFI-00000

Pull Request Formatting

Pull Request based on current revision of the main branch
Pull Request refers to a feature branch with one commit containing changes

Verification

Please indicate the verification steps performed prior to pull request creation.

Build

Build completed using mvn clean install -P contrib-check
- JDK 21

UI Contributions

NiFi is modernizing its UI. Any contributions that update the current UI also need to be implemented in the new UI.

Licensing

New dependencies are compatible with the Apache License 2.0 according to the License Policy
New dependencies are documented in applicable LICENSE and NOTICE files

Documentation

Documentation formatting appears as expected in rendered files

…ing flows

exceptionfactory

Thanks for putting this together @bbende! This looks great overall, I just noted a few minor syntax-level recommendations. All of the new class files appear to have extra padding in the license header, and a couple other very minor things.

.../nifi-github-extensions/src/main/java/org/apache/nifi/github/GitHubCreateContentRequest.java

...le/nifi-github-extensions/src/main/java/org/apache/nifi/github/GitHubFlowRegistryClient.java

...le/nifi-github-extensions/src/main/java/org/apache/nifi/github/VersionedComponentModule.java

...ndle/nifi-github-extensions/src/main/java/org/apache/nifi/github/GitHubRepositoryClient.java

...le/nifi-github-extensions/src/main/java/org/apache/nifi/github/GitHubFlowRegistryClient.java

…nstallation tokens

bbende · 2024-05-07T19:52:07Z

Thanks @exceptionfactory ! Pushed updates, let me know of anything else

simonbence

Thanks for the new registry client this is something I think many people waited for :) I have a couple of comments if you could look them through. I did not yet run the client but I am planning before closing the PR

...le/nifi-github-extensions/src/main/java/org/apache/nifi/github/GitHubFlowRegistryClient.java

simonbence · 2024-05-08T10:47:16Z

...le/nifi-github-extensions/src/main/java/org/apache/nifi/github/GitHubFlowRegistryClient.java

+        final String branch = flowLocation.getBranch();
+        final String filePath = getSnapshotFilePath(flowLocation);
+
+        final RegisteredFlowSnapshot existingSnapshot = getSnapshot(filePath, branch);


The NiFiRegistry implemnetation utilizes a NoSuchFlowException for the cases the flow could not ne find. I think it would be beneficial to have something similar.

Will look into it, but mentioned in other comments how we don't really know if it's the bucket, file, or branch that doesn't exist.

...ndle/nifi-github-extensions/src/main/java/org/apache/nifi/github/GitHubRepositoryClient.java

simonbence · 2024-05-08T11:50:01Z

...ndle/nifi-github-extensions/src/main/java/org/apache/nifi/github/GitHubRepositoryClient.java

+    }
+
+    private void throwPathOrBranchNotFound(final String path, final String branch) throws FlowRegistryException {
+        throw new FlowRegistryException("Path [" + path + "] or Branch [" + branch + "] not found");


Depending on which part of the path is not existing (bucket, flow, version), there are specific FlowRegistryException subclasses for the case. Also as the new concept "Branch" was added, it might worth to add a specific exception for that as well.

Thanks for pointing out the sub-classes, I wasn't aware of those. There might be a few cases where we could leverage one of them, but I don't think we can use it here because we don't actually know which part doesn't exist. It could be any one of the bucket directory, the snapshot file name, or the branch. Unless we make a lot of individual requests to check each piece, but I don't want to do that because then we end up making lots of remote calls to GitHub for one single operation, and during background synchronize of many versioned PGs, it will end up being thousands of calls to GitHub.

...ndle/nifi-github-extensions/src/main/java/org/apache/nifi/github/GitHubRepositoryClient.java

simonbence · 2024-05-08T11:55:52Z

...ework/nifi-web/nifi-web-api/src/main/java/org/apache/nifi/web/StandardNiFiServiceFacade.java

@@ -5357,6 +5355,8 @@ public RegisteredFlow registerVersionedFlow(final String registryId, final Regis
        }

        try {
+            final String generatedId = registry.generateFlowId(flow.getName());


This might be something I would prefer not to expose outside of the node. This should be an internal implementation detail for the registry node (or implementation). Is there any particular reason this leaves the boundaries of a registry?

Previously the framework generated a UUID in service facade before calling the registerFlow method in registry client, but for GitHub we don't want a UUID, so this just allows the framework to ask the client to generate the id, and the default behavior can return a UUID like before, and the GitHub impl can do something different.

- Populate storageLocation and implement isStorageLocationApplicable - Determine read/write permissions for the repo based on the client credentials - Use the permissions to populate bucket permissions - Change logic for creating default bucket to only happen if no other buckets and if write permissions - Verify permissions accordingly in all client methods based on read or write - Consolidate getFilenames and getDirectoryNames to helper method

bbende · 2024-05-08T20:39:40Z

@simonbence I pushed up some improvements based on your review, I think the permissions part is working much better now that I figured out how get the permissions that the client has to the repo, and can use them to set the bucket permissions.

Items addressed:

Populate storageLocation and implement isStorageLocationApplicable
Determine read/write permissions for the repo based on the client credentials
Use the permissions to populate bucket permissions
Change logic for creating default bucket to only happen if no other buckets and if write permissions
Verify permissions accordingly in all client methods based on read or write
Consolidate getFilenames and getDirectoryNames to helper method

exceptionfactory

Thanks for the great work on this new feature @bbende, and thanks for the detailed review @simonbence. I reviewed the latest updates and tested out the changes, looks good! +1 merging

NIFI-13105 Implement a FlowRegistryClient that use GitHub for version…

944a6d4

…ing flows

bbende requested review from simonbence and exceptionfactory May 7, 2024 17:22

Fix checkstyle error

9ccb903

exceptionfactory requested changes May 7, 2024

View reviewed changes

Address review feedback, add additional authentication type for app i…

47f022b

…nstallation tokens

simonbence reviewed May 8, 2024

View reviewed changes

Add unit test and minor refactoring to support testing

fa06204

exceptionfactory approved these changes May 9, 2024

View reviewed changes

exceptionfactory closed this in 7cd1a30 May 9, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NIFI-13105 Implement a FlowRegistryClient that use GitHub for version… #8765

NIFI-13105 Implement a FlowRegistryClient that use GitHub for version… #8765

bbende commented May 7, 2024

exceptionfactory left a comment

bbende commented May 7, 2024

simonbence left a comment

simonbence May 8, 2024

bbende May 8, 2024

simonbence May 8, 2024

bbende May 8, 2024

simonbence May 8, 2024

bbende May 8, 2024

bbende commented May 8, 2024 •

edited

exceptionfactory left a comment

NIFI-13105 Implement a FlowRegistryClient that use GitHub for version… #8765

NIFI-13105 Implement a FlowRegistryClient that use GitHub for version… #8765

Conversation

bbende commented May 7, 2024

Summary

Tracking

Issue Tracking

Pull Request Tracking

Pull Request Formatting

Verification

Build

UI Contributions

Licensing

Documentation

exceptionfactory left a comment

Choose a reason for hiding this comment

bbende commented May 7, 2024

simonbence left a comment

Choose a reason for hiding this comment

simonbence May 8, 2024

Choose a reason for hiding this comment

bbende May 8, 2024

Choose a reason for hiding this comment

simonbence May 8, 2024

Choose a reason for hiding this comment

bbende May 8, 2024

Choose a reason for hiding this comment

simonbence May 8, 2024

Choose a reason for hiding this comment

bbende May 8, 2024

Choose a reason for hiding this comment

bbende commented May 8, 2024 • edited

exceptionfactory left a comment

Choose a reason for hiding this comment

bbende commented May 8, 2024 •

edited