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

BIGTOP-4173 : DDL file to create all related tables (MySQL) #27

Merged
merged 8 commits into from
Jul 29, 2024

Conversation

HalimKim
Copy link
Contributor

No description provided.

@HalimKim HalimKim requested a review from kevinw66 July 28, 2024 15:20
@kevinw66
Copy link
Contributor

Looks like ci is failed, could you help fix it?

@HalimKim
Copy link
Contributor Author

Looks like ci is failed, could you help fix it?
@kevinw66

CI Test builds without SkipTests and uses postgres as a meta db.
looks like not using ddl file instead depending on JPA ddl auto generation.

There will be 2 possible resolutions

  1. fix the ci and test file to use mysql and the ddl file.
  2. leave ddl-generation 'create-or-extend-tables'.

what do you think? which one seems better?

@kevinw66
Copy link
Contributor

How about we add skip tests on ci.yml? we can add a separate job to run ut after we have some ut cases

@kevinw66
Copy link
Contributor

BTW, I think we need to add a sql to insert default admin user here, INSERT INTO user (id, username, password, nickname, status, create_time, update_time)VALUES (1, 'admin', '21232f297a57a5a743894a0e4a801fc3', 'Administrator', true, now(), now());

@HalimKim
Copy link
Contributor Author

@kevinw66
Thank you for sharing your idea.

I agree with your idea adding SkipTests and unit tests later. but for now, it will be really helpful that CI include build and test.(even just executing server) so I prefer leaving ddl-generation 'create-or-extend-tables' for now.

inserting default admin user will be good for testing, I will add another insert query.

@kevinw66
Copy link
Contributor

I agree with your idea adding SkipTests and unit tests later. but for now, it will be really helpful that CI include build and test.(even just executing server) so I prefer leaving ddl-generation 'create-or-extend-tables' for now.

Sounds good!

@@ -22,24 +22,49 @@
-- DROP DATABASE IF EXISTS `ambari`;
-- DROP USER `ambari`;
Copy link
Contributor

Choose a reason for hiding this comment

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

We can remove this


# USE @schema;
#
USE @schema;
Copy link
Contributor

Choose a reason for hiding this comment

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

New problems here after we reformat these lines

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops, my IDE did that i will correct this.

set
@minor = cast(substring_index(@version_short, '.', -1) as SIGNED);
set
@engine_stmt = IF((@major >= 5 AND @minor>=6) or @major >= 8, 'SET default_storage_engine=INNODB', 'SET storage_engine=INNODB');
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we shouldn't reformat these also

Copy link
Contributor

@kevinw66 kevinw66 left a comment

Choose a reason for hiding this comment

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

+1 LGTM, thanks @HalimKim

@kevinw66 kevinw66 merged commit d2a2dae into apache:main Jul 29, 2024
4 checks passed
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.

2 participants