Skip to content
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

[Task] All samples unified switch to annotation + yaml configuration … #1128

Closed

Conversation

cnzakii
Copy link

@cnzakii cnzakii commented Apr 7, 2024

What is the purpose of the change

[Task] All samples unified switch to annotation + yaml configuration #13860

Brief changelog

Convert all application.properties into application.yml

Verifying this change

Checklist

  • Make sure there is a GitHub_issue field for the change (usually before you start working on it). Trivial changes like typos do not require a GitHub issue. Your pull request should address just this issue, without pulling in other changes - one PR resolves one issue.
  • Each commit in the pull request should have a meaningful subject line and body.
  • Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
  • Check if is necessary to patch to Dubbo 3 if you are work on Dubbo 2.7
  • Write necessary unit-test to verify your logic correction, more mock a little better when cross module dependency exist. If the new feature or significant change is committed, please remember to add sample in dubbo samples project.
  • Add some description to dubbo-website project if you are requesting to add a feature.
  • GitHub Actions works fine on your own branch.
  • If this contribution is large, please follow the Software Donation Guide.

Copy link
Member

@AlbumenJ AlbumenJ left a comment

Choose a reason for hiding this comment

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

@CrazyHZM PTAL

@CrazyHZM
Copy link
Member

CrazyHZM commented Apr 8, 2024

@cnzakii Please fix the ci.

@cnzakii
Copy link
Author

cnzakii commented Apr 8, 2024

@CrazyHZM
The reasons causing the CI checks to fail do not appear to be caused by this PR:

  1. The CI check for Dubbo 3.2 passed without issues.
  2. The failures in Dubbo 3.3 / Integration Test (Java17, Job4) and Dubbo 3.3 / Integration Test (Java21, Job4) were due to timeouts in the dubbo-samples-callback module, but this PR did not modify that module.
    2024-04-08_21 08 52
  3. The failure in Dubbo 3.3 / Integration Test (Java21, Job3) was due to log4j-slf4j2-impl cannot be present with log4j-to-slf4j, which is not caused by the configuration files.
    2024-04-08_21 02 40

Comment on lines +16 to +31
dubbo:
application:
# Specify the application name of Dubbo
name: extensibility-filter-provider
protocol:
# Specify the port of Dubbo protocol
port: 20881
provider:
# Apply AppendedFilter
filter: appended
# Enable token verification for each invocation
token: true
registry:
# Specify the registry address
address: nacos://localhost:8848?username=nacos&password=nacos
# address: nacos://localhost:8848?username=nacos&password=nacos
Copy link
Member

Choose a reason for hiding this comment

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

Is this configuration file only a new, no-removed properties file?

Copy link
Author

@cnzakii cnzakii Apr 9, 2024

Choose a reason for hiding this comment

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

No, this is also replacing application.properties with application.yml
This repository is apache/dubbo (my PR is not merged):
image
image
This repository is cnzakii/dubbo (My own repository, merged with my commits):
image

Copy link
Member

Choose a reason for hiding this comment

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

@cnzakii My meaning is we should remove the application.properties.

Copy link
Author

Choose a reason for hiding this comment

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

@CrazyHZM
My reply above is explaining that I have deleted application.properties.
But I don’t know why it is not shown here that application.properties has been deleted

Copy link
Member

Choose a reason for hiding this comment

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

Ok, I checked your branch, and it is ok; it seems to be a GitHub issue.

@CrazyHZM
Copy link
Member

CrazyHZM commented Apr 9, 2024

@cnzakii For new contributors to the community, we would prefer to see as small a PR as possible for one or a few modules, as this is easier to review and can be merged faster.

@cnzakii
Copy link
Author

cnzakii commented Apr 9, 2024

@cnzakii For new contributors to the community, we would prefer to see as small a PR as possible for one or a few modules, as this is easier to review and can be merged faster.

OK, I get it, thanks for reminding me

@chickenlj
Copy link
Contributor

Thanks @cnzakii , we really appreciate your contribution. I agree with @CrazyHZM lease one module each time.

@cnzakii
Copy link
Author

cnzakii commented Apr 15, 2024

OK, I will close this "massive" pull request and submit new pull requests module by module.

@cnzakii cnzakii closed this Apr 15, 2024
@cnzakii cnzakii deleted the change-configuration-file-formate branch April 15, 2024 11:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants