-
-
Notifications
You must be signed in to change notification settings - Fork 388
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
Setting a database baseline #1439
Conversation
That's nice but, as I know, it won't work because baseline need at least Flyway Teams 😓 |
I tried it and it worked (with a additional migration script). So I hope maven use the Flyway baseline (command) which is included in the Flyway Community version. |
I didn't notice baseline command is available, thanks! I will check asap |
@fnkbsi I didn't check yet but if the baseline is working, you should be able to remove this lines without breaking the CI. Could you try? |
@fnkbsi For historical reason, I think the sources should include the V1_0_0 baseline. WDYT? |
Why should we change the privileges at the workflow script? There is no difference between the update and baseline scripts, the baseline scripts just replaces the update scripts up to the correspondent version number on a new installation. |
Setting the baseline at version 1_0_0 is also possible, just renaming the script V1_0_0 baseline.sql to B1_0_0 baseline.sql should do the trick. |
Because the extra rights were due to migrations before B1_0_0.
That was my point, could you add B1_0_0 too? |
If I add the B1_0_0 and don't remove the B1_0_5 the B1_0_0 script is dead code, because it will never be used. So I am hesitant to integrated it only for cosmetics. |
…GRANT SELECT ON mysql.proc TO 'steve'@'%';" -v || true"
hey there, i am up to speed with this change and discussion. sorry for the delay. first of all, thanks for the PR and insightful discussion! regarding where to set the baseline:
i am actually fine with both. i kinda like @juherr's suggestion a bit better because granularity of changes will be kept instead of one bulk monolith. my concern is these exports coming from heidi sql. i am not sure how it works, but i would rather prefer mysql's (or mariadb's) native dump functionality to prevent man-in-the-middle interpretations and probable errors. is this possible? or, is my concern even valid? wdyt? talking about mysql and mariadb... i see that the matrix builds were fine but, this baseline will be compatible with both, right? nuances started to appear in new mariadb versions because of which mariadb started to drift from mysql. |
Yes. I think major versions are better candidate for baselines. I've proposed an alternative pr #1455 which removes mysql privileges that are useless after v1.0 |
i was not following semantic versioning with db migration files though. these are just linear changes to be applied. my approach was:
example 1 : 0.8.8 -> 0.8.9 (fine) therefore, the baseline should and could be after the final change where the requirement for privileges is no more necessary. |
Funny versionning convention. I didn't get that before. Thanks for the clarification. In that case whatever version is used as baseline will be ok. About my PR, I made the baseline by hand and applied every fixes myself to be sure to respect the history. |
In general I agree, but V1_0_1 - V1_0_4 include only 'Alter Table' commands with minor changes.
There are differences in the exported/dumped scripts. On a first glance I've seen two or three general differences (except the comments).
Based on the github actions results, yes. Also I tested it locally with a mariadb v11 on my windows machine. |
…gs (executable comments); removed temporary tabels/views, because of that changed creation order of views transaction and ocpp_activity
…ecutable comments are changed to commands. except "/*!999999\- enable the sandbox mode */ "
FWIW, I pulled down this branch and attempted a docker compose up, but it failed:
|
SteVe doesn't support Java 11 anymore, minimum version is Java 17. You need to update the JDK in your docker |
if @leomwa made a fresh pull of the branch without modifications, it should be java 17 already. Dockerfile we use references java 17. therefore, i am confused since i dont know where this java 11 comes into play. |
@fnkbsi i see you made some changes after my comment. are these changes in reaction to my remarks? if yes, are you finished? is this PR stable now? |
@goekay Yes after your remarks I changed the base of the script to a mysqldump.exe exported script. The PR is stable unless there are more comments. |
thanks! migration file LGTM. i think you can even remove the |
Without the Super privileges the workflow does not run successful. MySql need SET_USER_ID privilege (https://github.com/fnkbsi/steve/actions/runs/9641524165) and mariadb struggles with the views without the Super privilege (https://github.com/fnkbsi/steve/actions/runs/9641825097). |
... because the view creation dictates
|
…definer statements in views
@fnkbsi should i merge or do you want to? i approved the PR to signal that you can merge it. |
@goekay: Please merge. |
thanks all! |
SUPER is not needed anymore
Some combination of DB versions and OS versions seems to have problems with the DB migration scripts.
@juherr suggested (#1394 (comment)) a baseline script to solve the issues (e.g. #1417).
The PR #1394 and #1428 addresses the same issues, but changes the existing migration scripts, which could causes trouble at updating existing Steve servers.
The baseline also improved slightly the build process, because less DB build/migration operation are executed.
The script is exported (by mariadb-dump) from a fresh build Steve instance. The baseline creates all tables and views and inserts the data of the setting table.