wmedvede commented on code in PR #1791: URL: https://github.com/apache/incubator-kie-kogito-images/pull/1791#discussion_r1696711121
########## modules/kogito-postgres-db-migration-deps/artifacts/migration.sh: ########## @@ -0,0 +1,73 @@ +#!/bin/bash +# +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +# + +if [ -z "$MIGRATE_DATA_INDEX" ]; then + MIGRATE_DATA_INDEX=true + echo "Migrating data index: $MIGRATE_DATA_INDEX" +fi + +if [ -z "$MIGRATE_JOBS_SERVICE" ]; then + MIGRATE_JOBS_SERVICE=true + echo "Migrating jobs service: $MIGRATE_JOBS_SERVICE" +fi + +if $MIGRATE_DATA_INDEX +then + echo DI DB ENV VARS Review Comment: All this particular exectuion of the migration itself, looks like a copy paste, we have same code or DI and JS, and only change the variables. I think there are room to extract this processing into a reusable script. ########## modules/kogito-postgres-db-migration-deps/artifacts/db-migration-files/data-index/V1.32.0__data_index_create.sql: ########## @@ -0,0 +1,307 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +SET SEARCH_PATH="$DATA_INDEX_SCHEMA"; Review Comment: By passing schema to flyway as a parameter -schemas=jobs-service, etc, the idea is to evict setting the schema at this point. I think we must remove the setting of this search path in all the .sql files. Also, consider that this .sql at some point, we must automate and copy from the original services sources. Right now, what we have is a manual copy of this files, however, this files really comes from the services definition, for example: https://repository.apache.org/content/groups/snapshots/org/kie/kogito/kogito-ddl/10.0.999-SNAPSHOT/kogito-ddl-10.0.999-20240726.011627-10-db-scripts.zip  :point_up: If you open this file, you'll see the files we have here don't have any schema. ########## modules/kogito-postgres-db-migration-deps/artifacts/migration.sh: ########## @@ -0,0 +1,73 @@ +#!/bin/bash +# +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +# + +if [ -z "$MIGRATE_DATA_INDEX" ]; then Review Comment: In my opinion this approach of automatically apply the migration if for example MIGRATE_DATA_INDEX is not set, is not the best option. Why? Because the migration is a serious thing, once applied, we have impact on the DB. And thus, from a system configurator perspective, you must be totally conscious of when you decide to include the migration. Basically by explicitly declaring MIGRATE_DATA_INDEX=yes (yes a want to migrate the DI) Whatever any other configuration, including a typo, MIGRATE_DATA_INDEX=yess, must not start any migration for the DI. For example, with the current scripts formulation look what happened in this case: docker run -it -e MIGRATE_DATA_INDEX_SERVICE=**falsee** \ <- **system administrator made a typo here** -e JS_DB_URL=jdbc:postgresql://192.168.1.117:5432/postgres \ -e JS_DB_USER=postgres \ -e JS_DB_PWD=pass \ docker.io/apache/incubator-kie-kogito-postgres-db-migration:999-SNAPSHOT The migration automatically starts by trying to migrate the DI, not good. DI DB ENV VARS URL= USER= PWD= Using the data index schema: data-index-service LISTING SQL DIR: DATA-INDEX V1.32.0__data_index_create.sql V1.45.0.0__data_index_node_definitions.sql V1.45.0.2__data_index_definitions_add_columns.sql V1.45.0.4__increase_varchar_length.sql V1.44.0__data_index_definitions.sql V1.45.0.1__add_identity_to_process_instance.sql V1.45.0.3__add_fk_index.sql A more recent version of Flyway is available. Find out more about Flyway 10.17.0 at https://rd.gt/3rXiSlV Flyway Community Edition 10.16.0 by Redgate See release notes here: https://rd.gt/416ObMi ERROR: Unable to connect to the database. Configure the url, user and password! Refer to the flyway.toml.example file in the /conf folder in the installation directory. A more recent version of Flyway is available. Find out more about Flyway 10.17.0 at https://rd.gt/3rXiSlV Flyway Community Edition 10.16.0 by Redgate See release notes here: https://rd.gt/416ObMi ERROR: Unable to connect to the database. Configure the url, user and password! Refer to the flyway.toml.example file in the /conf folder in the installation directory. ########## modules/kogito-postgres-db-migration-deps/artifacts/migration.sh: ########## @@ -0,0 +1,73 @@ +#!/bin/bash +# +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +# + +if [ -z "$MIGRATE_DATA_INDEX" ]; then + MIGRATE_DATA_INDEX=true + echo "Migrating data index: $MIGRATE_DATA_INDEX" +fi + +if [ -z "$MIGRATE_JOBS_SERVICE" ]; then + MIGRATE_JOBS_SERVICE=true + echo "Migrating jobs service: $MIGRATE_JOBS_SERVICE" +fi + +if $MIGRATE_DATA_INDEX +then + echo DI DB ENV VARS + echo URL=$DI_DB_URL USER=$DI_DB_USER PWD=$DI_DB_PWD + + if [ -z "$DATA_INDEX_SCHEMA" ]; then + DATA_INDEX_SCHEMA=data-index-service + echo "Using the data index schema: $DATA_INDEX_SCHEMA" + fi + + # Update $DATA_INDEX_SCHEMA for data index sql files + cd /home/default/db-migration-files/data-index + for FILE in *; do sed -i.bak 's/$DATA_INDEX_SCHEMA/'$DATA_INDEX_SCHEMA'/' $FILE; done + rm -rf *.bak + + echo LISTING SQL DIR: DATA-INDEX + ls /home/default/db-migration-files/data-index + + /home/default/flyway/flyway -url="$DI_DB_URL" -user="$DI_DB_USER" -password="$DI_DB_PWD" -mixed="true" -locations="filesystem:/home/default/db-migration-files/data-index" -schemas="$DATA_INDEX_SCHEMA" migrate + /home/default/flyway/flyway -url="$DI_DB_URL" -user="$DI_DB_USER" -password="$DI_DB_PWD" -mixed="true" -locations="filesystem:/home/default/db-migration-files/data-index" -schemas="$DATA_INDEX_SCHEMA" info +fi + +if $MIGRATE_JOBS_SERVICE +then + echo JS DB ENV VARS + echo URL=$JS_DB_URL USER=$JS_DB_USER PWD=$JS_DB_PWD Review Comment: I think that we shouldn't echo passwords, we can print ******** instead. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
