Skip to content

config: allow to call config:get() from app script #8929

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

Merged

Conversation

Totktonada
Copy link
Member

It is convenient to access configuration using config:get() from the application script (app.file or app.module).

However, before this commit, it was not possible, because the configuration was not considered as applied before the application script is loaded.

Now, the script loading is moved into the post-apply phase.

The resulting sequence of steps on startup/reload is the following.

  • collect configuration information (from all the sources)
  • <if the previous step is failed, set check_errors status and break>
  • apply the configuration (call all the appliers)
  • <if the previous step is failed, set check_errors status and break>
  • <set the new successful status: ready or check_warnings>
  • call post-apply hooks (including application script loading)
  • <if the previous step is failed, set check_errors status and break>
  • <set the new successful status: ready or check_warnings>

Part of #8862


See also a state diagram in tarantool/doc#3544.

@Totktonada Totktonada requested a review from ImeevMA July 28, 2023 18:40
@Totktonada Totktonada requested a review from a team as a code owner July 28, 2023 18:40
@Totktonada Totktonada requested a review from a team as a code owner July 28, 2023 18:40
@Totktonada Totktonada force-pushed the gh-8862-use-config-get-from-app-script branch from e67e9d1 to 171a568 Compare July 28, 2023 18:50
@coveralls
Copy link

coveralls commented Jul 28, 2023

Coverage Status

coverage: 86.1% (-0.03%) from 86.127% when pulling 13b84a0 on Totktonada:gh-8862-use-config-get-from-app-script into a6054f0
on tarantool:master
.

@Totktonada Totktonada force-pushed the gh-8862-use-config-get-from-app-script branch from 171a568 to 649132f Compare July 31, 2023 16:51
Copy link
Collaborator

@ImeevMA ImeevMA left a comment

Choose a reason for hiding this comment

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

Thank you for the patches! I have couple of non-blocking comments, feel free to ignore them. In general, I think init.lua is getting pretty complex.

Copy link
Contributor

@ylobankov ylobankov left a comment

Choose a reason for hiding this comment

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

Tests LGTM

The declarative config has the `app` section with following three
parameters:

* `app.file` -- the path to the script file
* `app.module` -- the same, but the script is searched using the Lua
  search paths (and loaded using `require`)
* `app.cfg` -- a user provided configuration (arbitrary map)

This commit adds a few success/failure cases. The goal is to have simple
test examples to add more ones easier in a future.

Implemented several test helpers for typical scenarios that can be used
outside of the application script test.

Part of tarantool#8862

NO_DOC=testing improvements
NO_CHANGELOG=see NO_DOC
It is convenient to access configuration using `config:get()` from the
application script (`app.file` or `app.module`).

However, before this commit, it was not possible, because the
configuration was not considered as applied before the application
script is loaded.

Now, the script loading is moved into the post-apply phase.

The resulting sequence of steps on startup/reload is the following.

* collect configuration information (from all the sources)
* <if the previous step is failed, set `check_errors` status and break>
* apply the configuration (call all the appliers)
* <if the previous step is failed, set `check_errors` status and break>
* <set the new successful status: `ready` or `check_warnings`>
* call post-apply hooks (including application script loading)
* <if the previous step is failed, set `check_errors` status and break>
* <set the new successful status: `ready` or `check_warnings`>

I would like to briefly comment the changes in the tests.

* `app_test.lua`: added the check for the new behavior (call
  config:get() from the app script)
* `appliers_test.lua`: fixed the applier call (`.apply` ->
  `.post_apply`)
* `config_test.lua`: fixed status observed in the app script
  (`*_in_progress` -> `ready`)

Part of tarantool#8862

NO_DOC=reflected in tarantool/doc#3544
@Totktonada Totktonada force-pushed the gh-8862-use-config-get-from-app-script branch from 649132f to 13b84a0 Compare August 2, 2023 23:43
@Totktonada Totktonada added the full-ci Enables all tests for a pull request label Aug 2, 2023
@Totktonada Totktonada merged commit 0cb9101 into tarantool:master Aug 3, 2023
@Totktonada Totktonada deleted the gh-8862-use-config-get-from-app-script branch August 3, 2023 07:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
full-ci Enables all tests for a pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants