NIFI-9288: Updated data model to use for cnofig verification requests… by markap14 · Pull Request #5452 · apache/nifi

markap14 · 2021-10-11T15:42:42Z

… so that only properties are necessary not the full component/config dtos. Also added endpoint necessary for determining which attributes are referenced by a component's properties and removed the referencedAttributes field from the compnoents themselves, since there's now a new endpoint for it. Also fixed a bug that was encountered where the VerifyConfigRequestDTO's complete flag was incorrect in case of failures.

Thank you for submitting a contribution to Apache NiFi.

Please provide a short description of the PR here:

Description of PR

Enables X functionality; fixes bug NIFI-YYYY.

In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:

For all changes:

Is there a JIRA ticket associated with this PR? Is it referenced
in the commit message?
Does your PR title start with NIFI-XXXX where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.
Has your PR been rebased against the latest commit within the target branch (typically main)?
Is your initial contribution a single, squashed commit? Additional commits in response to PR reviewer feedback should be made on this branch and pushed to allow change tracking. Do not squash or use --force when pushing to allow for clean monitoring of changes.

For code changes:

Have you ensured that the full suite of tests is executed via mvn -Pcontrib-check clean install at the root nifi folder?
Have you written or updated unit tests to verify your changes?
Have you verified that the full build is successful on JDK 8?
Have you verified that the full build is successful on JDK 11?
If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?
If applicable, have you updated the LICENSE file, including the main LICENSE file under nifi-assembly?
If applicable, have you updated the NOTICE file, including the main NOTICE file found under nifi-assembly?
If adding new Properties, have you added .displayName in addition to .name (programmatic access) for each of the new properties?

For documentation related changes:

Have you ensured that format looks appropriate for the output in which it is rendered?

Note:

Please ensure that once the PR is submitted, you check GitHub Actions CI for build issues and submit an update to your PR as soon as possible.

mcgilman · 2021-10-11T17:23:33Z

Will review...

gresockj

This refactor does make the config verification API look cleaner to me. Some minor comments, and then I plan to test out the behavior later this week.

gresockj · 2021-10-14T11:49:49Z

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

+        final Map<String, String> referencedAttributes = determineReferencedAttributes(properties, processorNode, parameterContext);
+
+        final ConfigurationAnalysisDTO dto = new ConfigurationAnalysisDTO();
+        dto.setComponentId(processorId);
+        dto.setProperties(properties);
+        dto.setReferencedAttributes(referencedAttributes);
+
+        final ConfigurationAnalysisEntity entity = new ConfigurationAnalysisEntity();
+        entity.setConfigurationAnalysis(dto);
+        return entity;


It seems that this whole section could be reused between the three analyzeXConfiguration methods.

Yeah that's a good catch. Will do so.

gresockj · 2021-10-14T11:51:19Z

...k/nifi-web/nifi-web-api/src/main/java/org/apache/nifi/web/api/ControllerServiceResource.java

+    )
+    public Response analyzeConfiguration(
+        @ApiParam(value = "The controller service id.", required = true) @PathParam("id") final String controllerServiceId,
+        @ApiParam(value = "The configuration verification request.", required = true) final ConfigurationAnalysisEntity configurationAnalysis) {


Perhaps "configuration analysis request"

gresockj · 2021-10-14T11:53:45Z

...framework/nifi-web/nifi-web-api/src/main/java/org/apache/nifi/web/api/ProcessorResource.java

+    )
+    public Response analyzeConfiguration(
+        @ApiParam(value = "The processor id.", required = true) @PathParam("id") final String processorId,
+        @ApiParam(value = "The processor configuration verification request.", required = true) final ConfigurationAnalysisEntity configurationAnalysis) {


"configuration analysis request"

gresockj · 2021-10-14T11:54:19Z

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

+    )
+    public Response analyzeConfiguration(
+        @ApiParam(value = "The reporting task id.", required = true) @PathParam("id") final String reportingTaskId,
+        @ApiParam(value = "The configuration verification request.", required = true) final ConfigurationAnalysisEntity configurationAnalysis) {


Same as above

gresockj · 2021-10-14T11:57:39Z

...cli/src/main/java/org/apache/nifi/toolkit/cli/impl/client/nifi/ControllerServicesClient.java


-    VerifyControllerServiceConfigRequestEntity deleteConfigVerificationRequest(String serviceId, String verificationRequestId) throws NiFiClientException, IOException;
+    VerifyConfigRequestEntity deleteConfigVerificationRequest(String serviceId, String verificationRequestId) throws NiFiClientException, IOException;



What do you think about adding submitConfigAnalysisRequest here and in the other clients in order to support some new system tests?

I think adding that in, along with system tests is definitely a good idea. But I don't think we should block this PR in order to provide system tests. And I'd recommend not implementing that in the client until we build the system tests that can exercise & verify the behavior. It's not immediately clear to me how this might be used from the client outside of the context of system tests, so I'd recommend we implement it in a future PR.

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

… so that only properties are necessary not the full component/config dtos. Also added endpoint necessary for determining which attributes are referenced by a component's properties and removed the referencedAttributes field from the compnoents themselves, since there's now a new endpoint for it. Also fixed a bug that was encountered where the VerifyConfigRequestDTO's complete flag was incorrect in case of failures.

…mponent, it populates default values instead of returning null for default values. Also ignore any properties that don't support expression language when determining which attributes are referenced

markap14 · 2021-10-18T13:39:37Z

Thanks for the review @gresockj - addressed comments and rebased against main so used a force push. Thanks!

mcgilman

Thanks for the PR @markap14! Been running this PR for the last couple of weeks while building the corresponding front-end pieces. Looks great! Will have the front-end PR posted soon.

gresockj

Thanks for the changes, @markap14 ! I verified that the functionality still works after the latest commit. The system test failures don't look related, so I'll go ahead and merge if the main build checks look clean.

… so that only properties are necessary not the full component/config dtos. Also added endpoint necessary for determining which attributes are referenced by a component's properties and removed the referencedAttributes field from the compnoents themselves, since there's now a new endpoint for it. Also fixed a bug that was encountered where the VerifyConfigRequestDTO's complete flag was incorrect in case of failures Signed-off-by: Joe Gresock <jgresock@gmail.com> This closes apache#5452.

markap14 requested a review from mcgilman October 11, 2021 15:42

mcgilman added the hacktoberfest-accepted Hacktoberfest Accepted label Oct 11, 2021

gresockj self-requested a review October 14, 2021 11:38

gresockj requested changes Oct 14, 2021

View reviewed changes

gresockj reviewed Oct 16, 2021

View reviewed changes

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

markap14 added 3 commits October 18, 2021 09:26

NIFI-9288: Ensure that when we get the configured properties for a co…

6ecf22a

…mponent, it populates default values instead of returning null for default values. Also ignore any properties that don't support expression language when determining which attributes are referenced

NIFI-9288: Addressed review feedback

a7fa5d9

markap14 force-pushed the NIFI-9288 branch from 707f441 to a7fa5d9 Compare October 18, 2021 13:39

mcgilman approved these changes Oct 18, 2021

View reviewed changes

gresockj approved these changes Oct 18, 2021

View reviewed changes

asfgit closed this in 151a936 Oct 18, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NIFI-9288: Updated data model to use for cnofig verification requests…#5452

NIFI-9288: Updated data model to use for cnofig verification requests…#5452
markap14 wants to merge 3 commits intoapache:mainfrom
markap14:NIFI-9288

markap14 commented Oct 11, 2021

Uh oh!

mcgilman commented Oct 11, 2021

Uh oh!

gresockj left a comment

Uh oh!

gresockj Oct 14, 2021

Uh oh!

markap14 Oct 18, 2021

Uh oh!

gresockj Oct 14, 2021

Uh oh!

gresockj Oct 14, 2021

Uh oh!

gresockj Oct 14, 2021

Uh oh!

gresockj Oct 14, 2021

Uh oh!

markap14 Oct 18, 2021

Uh oh!

Uh oh!

markap14 commented Oct 18, 2021

Uh oh!

mcgilman left a comment

Uh oh!

gresockj left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants


		VerifyControllerServiceConfigRequestEntity deleteConfigVerificationRequest(String serviceId, String verificationRequestId) throws NiFiClientException, IOException;
		VerifyConfigRequestEntity deleteConfigVerificationRequest(String serviceId, String verificationRequestId) throws NiFiClientException, IOException;

Conversation

markap14 commented Oct 11, 2021

Description of PR

For all changes:

For code changes:

For documentation related changes:

Note:

Uh oh!

mcgilman commented Oct 11, 2021

Uh oh!

gresockj left a comment

Choose a reason for hiding this comment

Uh oh!

gresockj Oct 14, 2021

Choose a reason for hiding this comment

Uh oh!

markap14 Oct 18, 2021

Choose a reason for hiding this comment

Uh oh!

gresockj Oct 14, 2021

Choose a reason for hiding this comment

Uh oh!

gresockj Oct 14, 2021

Choose a reason for hiding this comment

Uh oh!

gresockj Oct 14, 2021

Choose a reason for hiding this comment

Uh oh!

gresockj Oct 14, 2021

Choose a reason for hiding this comment

Uh oh!

markap14 Oct 18, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

markap14 commented Oct 18, 2021

Uh oh!

mcgilman left a comment

Choose a reason for hiding this comment

Uh oh!

gresockj left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants