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

fix(db/migration): do migration before validation #10348

Merged
merged 5 commits into from
Feb 23, 2023

Conversation

StarlightIbuki
Copy link
Contributor

@StarlightIbuki StarlightIbuki commented Feb 22, 2023

Summary

We choose to only handle nesting in top-level services.

Checklist

  • The Pull Request has tests
  • There's an entry in the CHANGELOG

N/A There is a user-facing docs PR against https://github.com/Kong/docs.konghq.com - PUT DOCS PR HERE

Issue reference

Fix KAG-638

@chronolaw chronolaw changed the title fix(dbless): migration before validation fix(db/migration): do migration before validation Feb 22, 2023
@chronolaw chronolaw linked an issue Feb 22, 2023 that may be closed by this pull request
1 task
@StarlightIbuki StarlightIbuki force-pushed the fix/on-the-fly-mig-validation branch from ae59e39 to 1acd47f Compare February 23, 2023 02:51
if not routes then
-- no need to migrate
return
local function table_default(val)
Copy link
Contributor

Choose a reason for hiding this comment

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

table_or_default or get_valid_table may be a better name.

@chronolaw
Copy link
Contributor

Perhaps we should cherry-pick this PR to the 3.2 release branch.

We choose to only handle nesting in top-level services.

Fix KAG-638

Co-authored-by: Chrono <chrono_cpp@me.com>

fix lint
@StarlightIbuki StarlightIbuki force-pushed the fix/on-the-fly-mig-validation branch from 7e75cbb to 227b5ee Compare February 23, 2023 04:41
CHANGELOG.md Outdated Show resolved Hide resolved
Co-authored-by: Chrono <chrono_cpp@me.com>
@StarlightIbuki StarlightIbuki added this to the 3.2.1 milestone Feb 23, 2023
Copy link
Member

@bungle bungle left a comment

Choose a reason for hiding this comment

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

Some comments, am I right about these?

@@ -82,6 +82,8 @@
[#10346](https://github.com/Kong/kong/pull/10346)
- Fix an issue where control plane does not rename fields correctly for `session` for older version of data planes.
[#10352](https://github.com/Kong/kong/pull/10352)
- Fix an issue where validation to regex routes may be skipped when the old-fashioned config is used for DB-less Kong.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- Fix an issue where validation to regex routes may be skipped when the old-fashioned config is used for DB-less Kong.
- Fix an issue where migration to regex routes may be skipped when the old-fashioned config is used for DB-less Kong.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No. It's not the migration being skipped. The migration will happen anyway.

Copy link
Member

@bungle bungle Feb 23, 2023

Choose a reason for hiding this comment

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

So the issue is about validation being skipped? Aka it should fail validation, but now it does not fail? I tought it was about migrating also service nested routes that was not done before.

kong/db/declarative/migrations/route_path.lua Outdated Show resolved Hide resolved
return
local function table_default(val)
-- we cannot verify it with type(val) == "table" because
-- we may recieve indexable cdata/lightuserdata
Copy link
Member

Choose a reason for hiding this comment

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

-- we may recieve indexable cdata/lightuserdata

When and why would this happen?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The caller may pass an object representing yaml/json object, which could be wrapped c object.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For the same reason, I'm using pairs instead of pairs. But let's give it a try and revert if it fails.

StarlightIbuki and others added 2 commits February 23, 2023 19:14
Co-authored-by: Aapo Talvensaari <aapo.talvensaari@gmail.com>
Co-authored-by: Aapo Talvensaari <aapo.talvensaari@gmail.com>
@kikito kikito merged commit 2f82ced into master Feb 23, 2023
@kikito kikito deleted the fix/on-the-fly-mig-validation branch February 23, 2023 17:58
mashapedeployment pushed a commit that referenced this pull request Feb 24, 2023
Co-authored-by: Chrono <chrono_cpp@me.com>
Co-authored-by: Aapo Talvensaari <aapo.talvensaari@gmail.com>
Co-authored-by: Enrique García Cota <kikito@gmail.com>
(cherry picked from commit 2f82ced)
dndx pushed a commit that referenced this pull request Feb 28, 2023
Co-authored-by: Chrono <chrono_cpp@me.com>
Co-authored-by: Aapo Talvensaari <aapo.talvensaari@gmail.com>
Co-authored-by: Enrique García Cota <kikito@gmail.com>
(cherry picked from commit 2f82ced)
This was referenced Mar 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Problems loading router with regex in kong 3x
4 participants