wmedvede commented on code in PR #1786: URL: https://github.com/apache/incubator-kie-kogito-images/pull/1786#discussion_r1689558938
########## modules/kogito-postgres-db-migration-deps/artifacts/db-migration-files/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-service"; Review Comment: In general, I'd say that we can't hard code the namespace here. If you look in the original scripts we never do. Same comment apply for all the .sql files. ########## modules/kogito-postgres-db-migration-deps/artifacts/migration.sh: ########## @@ -0,0 +1,25 @@ +#!/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. +# + +echo LISTING SQL DIR +ls /home/default/db-migration-files + +/home/default/flyway/flyway -url="$1" -user="$2" -password="$3" -mixed="true" -locations="filesystem:/home/default/db-migration-files" -schemas="public,data-index-service,jobs-service" migrate Review Comment: For the parameters user, password, or any other that user must provide, I think we should use env var names instead. For example URL, USER, PASSWORD, and then, we can do -user="$URL" instead. Then we can document these parameters, and the image will be much more usable. for example, people can execute the image by doing: docker run -it -e URK=myurl -e USER=myuser -e PASSWORD=xxxx apache/incubator-kie-kogito-postgres-db-migration-deps This will also be useful when we use the image in k8n ########## kogito-postgres-db-migration-image.yaml: ########## @@ -0,0 +1,45 @@ +# +# 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. +# +name: "docker.io/apache/incubator-kie-kogito-postgres-db-migration" +version: "999-SNAPSHOT" +from: --platform=linux/x86_64 registry.access.redhat.com/ubi8/openjdk-17-runtime:1.19 Review Comment: This from: line doesn't work in docker. Also for the rest of the images we have only from: "registry.access.redhat.com/ubi8/openjdk-17-runtime:1.19", I think we must align with the rest. ########## modules/kogito-postgres-db-migration-deps/module.yaml: ########## @@ -0,0 +1,30 @@ +# +# 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. +# +schema_version: 1 +name: kogito-postgres-db-migration-deps +version: "1.0" +artifacts: + - name: db-migration-files + path: artifacts/db-migration-files Review Comment: I think we must split the DB migration files in two sub-directories, jobs-service and data-index. This will let us have more control over the migrations in situations where we don't want to migrate both services at the same time, or even in situations where the services are in different databases. ########## modules/kogito-postgres-db-migration-deps/artifacts/migration.sh: ########## @@ -0,0 +1,25 @@ +#!/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. +# + +echo LISTING SQL DIR +ls /home/default/db-migration-files + +/home/default/flyway/flyway -url="$1" -user="$2" -password="$3" -mixed="true" -locations="filesystem:/home/default/db-migration-files" -schemas="public,data-index-service,jobs-service" migrate Review Comment: Regarding the migration (and regarding the comment on splitting the migration scripts), I think we can't execute the migration on the assumption that DI and JS are installed on the same DB, and in fixed namespace names. I think we must cover the following situations. 1) DI and JS are in different databases 2) The namespace for each service is user configurable. 3) Maybe in an installation only DI is present and not JS at all. So, I think we should do something like this, or a convenient variant. We must have for instance two parameters: MIGRATE_DATA_INDEX=true MIGRATE_JOBS_SERVICE=true (or a condensed variant MIGRATE="data-index,jobs-service" , MIGRATE="jobs-service") So a at the time of execution of the migration we apply two independent migrations, and we invoke the flyway command separately for each service depending on the configuration. And, complementary, me must have different connection parameters for each service. DATA_INDEX_USER DATA_INDEX_PASSWORD DATA_INDEX_URL (with jdbc we can make the connection already point to a particular schema. We must see here how this works in combination with DATA_INDEX_SCHEMA.) DATA_INDEX_SCHEMA and similar to the jobs service. So, in the end, we apply separated migrations on demand. -- 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]
