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

validate the db name for all three moudles #4496

Closed
wants to merge 1 commit into from
Closed

validate the db name for all three moudles #4496

wants to merge 1 commit into from

Conversation

givemesomefaces
Copy link

@givemesomefaces givemesomefaces commented Jul 31, 2022

What's the purpose of this PR

validate the db name for all three moudles

Which issue(s) this PR fixes:

Fixes #4230

Brief changelog

If the database names of admin service and config service are missing 'config', or if the database name of portal service is missing 'portal', the startup will fail.

Follow this checklist to help us incorporate your contribution quickly and easily:

  • Read the Contributing Guide before making this pull request.
  • Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
  • Write necessary unit tests to verify the code.
  • Run mvn clean test to make sure this pull request doesn't break anything.
  • Update the CHANGES log.

@github-actions
Copy link

github-actions bot commented Jul 31, 2022

CLA Assistant Lite bot:
Thank you for your submission, we really appreciate it. Like many open-source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution. You can sign the CLA by just posting a Pull Request Comment same as the below format.


I have read the CLA Document and I hereby sign the CLA


You can retrigger this bot by commenting recheck in this Pull Request

@givemesomefaces givemesomefaces changed the title validate the db name for all three moudles (#4230) validate the db name for all three moudles Jul 31, 2022
}

private void checkDataBaseName() {
String checkFlag = applicationContext.getEnvironment().getProperty("config.check.databasename");
Copy link
Member

Choose a reason for hiding this comment

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

Shall we change the property name to apollo.check.database-name?
And is it better to use the ConditionalOnProperty mechanism? e.g.

@ConditionalOnProperty(value = {"apollo.check.database-name"}, havingValue = "true", matchIfMissing = true)

Copy link
Author

Choose a reason for hiding this comment

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

OK, I will revise it in this way.

Copy link
Author

Choose a reason for hiding this comment

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

I want to add logic to check the database name before creating the data source.
If use the @ConditionalOnProperty to deal with it, there will be a problem. There is a dependency in admin service,
adminServiceAuthenticationFilter -> bizConfig -> bizDBPropertySource -> serverConfigRepository -> serverConfig -> entityManager -> dataSourceScriptDatabaseInitializer -> dataSource, when the servlet is initialized, the filter will be initialized first,this will cause the data source to initialize, so unable to add name check before data source initialization. it seems meaningless if the name check is added after the data source is initialized,or is it the wrong way I handled it?

@github-actions github-actions bot locked and limited conversation to collaborators Aug 13, 2022
@givemesomefaces givemesomefaces deleted the dev_dbname_validate branch August 13, 2022 09:15
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

启动apollo-adminservice,报错,Unknown column 'serverconf0_.Cluster' in 'field list'
2 participants