Skip to content

NIFI-10679 MiNiFi C2 Update Asset Command#6583

Closed
briansolo1985 wants to merge 3 commits intoapache:mainfrom
briansolo1985:NIFI-10679_MINIFI_C2_UPDATE_ASSET
Closed

NIFI-10679 MiNiFi C2 Update Asset Command#6583
briansolo1985 wants to merge 3 commits intoapache:mainfrom
briansolo1985:NIFI-10679_MINIFI_C2_UPDATE_ASSET

Conversation

@briansolo1985
Copy link
Contributor

@briansolo1985 briansolo1985 commented Oct 26, 2022 •
edited
Loading

Summary

NIFI-10679

Tracking

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

Issue Tracking

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 8
    • JDK 11
    • JDK 17

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

@ferencerdei ferencerdei added the minifi Pull requests that updates minifi/c2 codes label Oct 26, 2022
@ferencerdei ferencerdei self-requested a review October 27, 2022 09:40
@bejancsaba bejancsaba self-requested a review October 27, 2022 12:31
bejancsaba
bejancsaba reviewed Oct 27, 2022
View reviewed changes
Copy link
Contributor

@bejancsaba bejancsaba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your change. It looks really good and is well covered with tests. I had one question around C2ClientConfig / properties could you please take a look there?

...nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/c2/C2NifiClientService.java
.c2Url(properties.getProperty(C2NiFiProperties.C2_REST_URL_KEY, ""))
.c2RequestCompression(properties.getProperty(C2NiFiProperties.C2_REQUEST_COMPRESSION_KEY, C2NiFiProperties.C2_REQUEST_COMPRESSION))
.confDirectory(properties.getProperty(C2NiFiProperties.C2_CONFIG_DIRECTORY_KEY, DEFAULT_CONF_DIR))
.confDirectory(properties.getProperty(C2NiFiProperties.C2_CONFIG_DIRECTORY_KEY, DEFAULT_CONF_DIR))
Copy link
Contributor

@bejancsaba bejancsaba Oct 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The asset directory shouldn't be added to the C2ClientConfig? I mean if we treat it as a c2 parameter I think it would make sense. What do you think?

Copy link
Contributor Author

@briansolo1985 briansolo1985 Oct 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, fixed it, it also became cleaner

...nifi-framework-core/src/main/java/org/apache/nifi/c2/command/TransferDebugCommandHelper.java
import java.util.stream.Stream;
import org.apache.nifi.util.NiFiProperties;

public class TransferDebugCommandHelper {
Copy link
Contributor

@bejancsaba bejancsaba Oct 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks it is cleaner this way!

Copy link
Contributor Author

@briansolo1985 briansolo1985 Oct 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks

ferencerdei
ferencerdei approved these changes Oct 28, 2022
View reviewed changes
Copy link
Contributor

@ferencerdei ferencerdei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this feature, +1 from me

@briansolo1985 briansolo1985 requested review from bejancsaba and ferencerdei and removed request for bejancsaba October 28, 2022 12:55
@briansolo1985
Copy link
Contributor Author

briansolo1985 commented Oct 28, 2022

I applied @bejancsaba 's recommendations, please check the PR again. Many thanks

bejancsaba
bejancsaba approved these changes Oct 28, 2022
View reviewed changes
Copy link
Contributor

@bejancsaba bejancsaba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 from my side, looks really good. Thank you.

priyanka-28 pushed a commit to priyanka-28/nifi that referenced this pull request Nov 16, 2022
@briansolo1985
NIFI-10679 MiNiFi C2 Update Asset Command
98899f7 
Signed-off-by: Csaba Bejan <bejan.csaba@gmail.com>

This closes apache#6583.
lizhizhou pushed a commit to lizhizhou/nifi that referenced this pull request Jan 2, 2023
@briansolo1985 @lizhizhou
NIFI-10679 MiNiFi C2 Update Asset Command
898efb8 
Signed-off-by: Csaba Bejan <bejan.csaba@gmail.com>

This closes apache#6583.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bejancsaba bejancsaba bejancsaba approved these changes

@ferencerdei ferencerdei Awaiting requested review from ferencerdei

Assignees

No one assigned

Labels

minifi Pull requests that updates minifi/c2 codes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@briansolo1985 @bejancsaba @ferencerdei