Skip to content

Feauture/convert xml to java conf#5525

Closed
Akvel wants to merge 3 commits intoapache:mainfrom
Akvel:feauture/convert-xml-to-java-conf
Closed

Feauture/convert xml to java conf#5525
Akvel wants to merge 3 commits intoapache:mainfrom
Akvel:feauture/convert-xml-to-java-conf

Conversation

@Akvel
Copy link
Copy Markdown

@Akvel Akvel commented Sep 28, 2021

Description

This PR convert Spring XML based configurations to Java-Configurations.

Converted beans that can be easily converted by scripts. I think this PR could be first step to Java-based configuration in all project.

Types of changes

  • Breaking change (fix or feature that would cause existing functionality to change)
  • New feature (non-breaking change which adds functionality)
  • Bug fix (non-breaking change which fixes an issue)
  • Enhancement (improves an existing feature and functionality)
  • Cleanup (Code refactoring and cleanup, that may add test cases)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

  • Major
  • Minor

How Has This Been Tested?

mvn test

@weizhouapache
Copy link
Copy Markdown
Member

@blueorangutan package

@blueorangutan
Copy link
Copy Markdown

@weizhouapache a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

Copy link
Copy Markdown
Contributor

@DaanHoogland DaanHoogland left a comment

Choose a reason for hiding this comment

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

quite a great effort. I wouldn't mind this excpet that the removed licenses are probably not acceptable in apache by-laws. Also can you justify this chance with a bit more text, please?

@@ -1,51 +1,10 @@
<!--
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

removing the license is not acceptable in apache foundation policy

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

fixed, also added to all new java classes

@blueorangutan
Copy link
Copy Markdown

Packaging result: ✖️ el7 ✖️ el8 ✖️ debian ✖️ suse15. SL-JID 1437

@@ -0,0 +1,84 @@
<!--
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This backup file can be excluded

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

fixed

@@ -0,0 +1,32 @@
<!--
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this backup file can be excluded

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

fixed

@@ -0,0 +1,32 @@
<!--
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

same here

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

fixed

@DaanHoogland
Copy link
Copy Markdown
Contributor

@Akvel thanks for joining in, can you add some context for us to understand your morives behind this PR, please? Especially because it is not eliminating the XML completely, what are we gaining going ahead with this?

@Akvel
Copy link
Copy Markdown
Author

Akvel commented Sep 29, 2021

thanks for joining in, can you add some context for us to understand your morives behind this PR, please? Especially because it is not eliminating the XML completely, what are we gaining going ahead with this?

@DaanHoogland I am converted beans that can be easily converted by scripts. I think this PR could be first step to Java-based configuration in all project.

@DaanHoogland
Copy link
Copy Markdown
Contributor

thanks @Akvel can you add that explanation to the description of the PR, please?
Also, such a decision should be discussed in a wider audience, so cc @weizhouapache @RodrigoDLopez @rhtyd @GabrielBrascher @sureshanaparti @ravening @GutoVeronezi . I really shouldn't cc so many people at once but the subject merits it.

@Akvel
Copy link
Copy Markdown
Author

Akvel commented Sep 30, 2021

thanks @Akvel can you add that explanation to the description of the PR, please? Also, such a decision should be discussed in a wider audience, so cc @weizhouapache @RodrigoDLopez @rhtyd @GabrielBrascher @sureshanaparti @ravening @GutoVeronezi . I really shouldn't cc so many people at once but the subject merits it.

done

@RodrigoDLopez
Copy link
Copy Markdown
Contributor

Thanks for the PR @Akvel. I liked the proposal.
But like everyone else who will be participating in this here, I have my fears about community approval.
I believe that starting a vote on the email list is the best way (with arguments that reinforce the need and, if necessary, a contextualization of the problem). As soon as the community approves, we return to the PR and discuss the possible points raised in the vote.

@ravening
Copy link
Copy Markdown
Member

@DaanHoogland will test it out soon

@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 7, 2022

Hi @${author}, your pull request has merge conflicts. Can you fix the conflicts and sync your branch with the base branch?

@DaanHoogland
Copy link
Copy Markdown
Contributor

@Akvel @ravening are you guys still on this?

@DaanHoogland DaanHoogland modified the milestones: 4.19.0.0, unplanned Nov 10, 2023
@DaanHoogland
Copy link
Copy Markdown
Contributor

@Akvel et al, closing this one as it has conflicts and is old. please update and reopen if you think it is still relevant.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants