Skip to content
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

"bin/kong migrate" should be embedded into "bin/kong start" #68

Closed
subnetmarco opened this issue Mar 11, 2015 · 4 comments
Closed

"bin/kong migrate" should be embedded into "bin/kong start" #68

subnetmarco opened this issue Mar 11, 2015 · 4 comments
Assignees

Comments

@subnetmarco
Copy link
Member

The bin/kong migrate command should be embedded into bin/kong start and it should try to migrate the database to the latest version.

If a database already exists, then bin/kong start should alert the user asking him to migrate with a message like:

Kong cannot start because your database needs to be migrate. Would you like to start a migration to a new schema (y/n)?

In this way we're making the first execution less painful, and we're also alerting the user when a newer version of Kong is running against and older schema without incurring in runtime exceptions later on.

@thibaultcha
Copy link
Member

  • Agree for running the migration the first time.
  • Agree for the message saying the migration is not up to date
  • Disagree for the [y/n] quick migration process. A migration is something that should not happen too often. It is something a user should really think through: backup the data, shutdown or maintenance-mode his kong cluster, run, check if good, bring back up. Emphasis on backup the data because I think it's the most important one and will be required even for single-node, non-cassandra clusters in the future.

@subnetmarco
Copy link
Member Author

Regarding you third point, I agree. In the future, we could treat potential breaking migrations differently. For example a potential breaking migration is something that updates an existing table and the user should be careful doing it. But on the other side a migration that just adds new tables to the keyspace won't do any harm to the existing data, so we should still notify the user but we can let him execute the migration after showing a [y/n] prompt.

@thibaultcha
Copy link
Member

Maybe later, but I think it's out of scope at this point. And I still don't think we should quick run migrations anyways. Big or small ones.

Let's make quick easy first migration, and an error if not migrated to the latest available. And pretty print any error. That's in the scope of the current state of Kong, being in beta etc. Will close this issue once done.

@subnetmarco subnetmarco added this to the 0.1.0-beta milestone Mar 11, 2015
thibaultcha added a commit that referenced this issue Mar 12, 2015
Instead of doing this in the bash script, we do it in main.lua
ini_by_lua since the error handling from bin/kong is already handled.

We only run migrations if we detect than they wer enever ran before. We
won't automatically run any new migration because the process shouldn't
be transparent, users should know about it and backup their data if they
want to.
@subnetmarco
Copy link
Member Author

Ok I agree. Let's implement the first two points.

thibaultcha added a commit that referenced this issue Mar 12, 2015
Instead of doing this in the bash script, we do it in main.lua
ini_by_lua since the error handling from bin/kong is already handled.

We only run migrations if we detect than they wer enever ran before. We
won't automatically run any new migration because the process shouldn't
be transparent, users should know about it and backup their data if they
want to.
ctranxuan pushed a commit to streamdataio/kong that referenced this issue Aug 25, 2015
Instead of doing this in the bash script, we do it in main.lua
ini_by_lua since the error handling from bin/kong is already handled.

We only run migrations if we detect than they wer enever ran before. We
won't automatically run any new migration because the process shouldn't
be transparent, users should know about it and backup their data if they
want to.
gszr pushed a commit that referenced this issue Oct 26, 2021
Fixes #68

The plugin assumed that certain variables such as `ctx.KONG_ACCESS_START` were always set. There are occasions where this isn't true, for example an auth plugin might skip the access phase altogether.

The assumption provoked unwanted status reports. In #68 we can see an example in which the expected status code is 400 but because of this assumption we get 500 instead.

This wraps all accesses to ctx.KONG_* with conditionals, using other values such as ngx_now_mu() if they are not present.

There is some repetitiveness, but I resisted abstracting into a function to allow for short-circuiting the call to obtain the default value (i.e. to only call to ngx_now() when it was really needed).
gszr pushed a commit that referenced this issue Oct 27, 2021
Fixes #68

The plugin assumed that certain variables such as `ctx.KONG_ACCESS_START` were always set. There are occasions where this isn't true, for example an auth plugin might skip the access phase altogether.

The assumption provoked unwanted status reports. In #68 we can see an example in which the expected status code is 400 but because of this assumption we get 500 instead.

This wraps all accesses to ctx.KONG_* with conditionals, using other values such as ngx_now_mu() if they are not present.

There is some repetitiveness, but I resisted abstracting into a function to allow for short-circuiting the call to obtain the default value (i.e. to only call to ngx_now() when it was really needed).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants