update kfp-api's apiserver configuration #375
Conversation
This: * removes deprecated `DBCONFIG_USER`, etc, environment variables (they have been replaced by variables of name `DBCONFIG_[driver]CONFIG_*`) * adds `OBJECTSTORECONFIG_HOST`, `_PORT`, and `_REGION`, which previously were required. Although currently they seem to be ignored due to kubeflow/pipelines#9689 - but in theory they'll matter again? Not sure exactly the scope of that issue.
github-actions
bot
added
Libraries: OK
Libraries: Out of sync
and removed
Libraries: OK
labels
github-actions
bot
added
Libraries: Out of sync
Libraries: OK
and removed
Libraries: OK
Libraries: Out of sync
labels
github-actions
bot
added
Libraries: Out of sync
Libraries: OK
and removed
Libraries: OK
Libraries: Out of sync
labels
There was a problem hiding this comment.
Thanks @ca-scribner, this change actually makes sense. As we tested it, the OBJECTSTORECONFIG_HOST and OBJECTSTORECONFIG_PORT env variables will actually be used by the apiserver when initialising the minio client.
I just left a minor comment to update the comments in the charm code to reflect what is actually happening.
NOTE: the integration tests are flaky at the moment, no need to block this PR because of them. Everything should be fixed after we do a last pass before merging to main.
charms/kfp-api/src/charm.py
Outdated
| # Configurations charmed-kubeflow adds to those of upstream | ||
| "ARCHIVE_CONFIG_LOG_FILE_NAME": self.model.config["log-archive-filename"], | ||
| "ARCHIVE_CONFIG_LOG_PATH_PREFIX": self.model.config["log-archive-prefix"], | ||
| # OBJECTSTORECONFIG_HOST and _PORT currently have no effect due to |
There was a problem hiding this comment.
Could we change this comment to say that these variables will actually have an effect and that they will be used in the absence of other env variables like MINIO_SERVICE_SERVICE_HOST and a missing key in the config.json? (related to #367 last two comments)
github-actions
bot
added
Libraries: OK
Libraries: Out of sync
and removed
Libraries: Out of sync
Libraries: OK
labels
* update kfp-api's apiserver configuration This: * removes deprecated `DBCONFIG_USER`, etc, environment variables (they have been replaced by variables of name `DBCONFIG_[driver]CONFIG_*`) * adds `OBJECTSTORECONFIG_HOST`, `_PORT`, and `_REGION`, which previously were required. Although currently they seem to be ignored due to kubeflow/pipelines#9689 - but in theory they'll matter again? Not sure exactly the scope of that issue.
This PR:
DBCONFIG_USER, etc, environment variables (they have been replaced by variables of nameDBCONFIG_[driver]CONFIG_*)OBJECTSTORECONFIG_HOST,_PORT, and_REGION, which previously were required. Although currently they seem to be ignored due to [feature] remove hard-coded configs in KFP V2 launcher kubeflow/pipelines#9689 - but in theory they'll matter again? Not sure exactly the scope of that issue.afaict, these are both helpful changes. The first doesn't actually affect anything, but removes unnecessary configs. The second should affect things, if not for kubeflow/pipelines#9689
May close #367