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

feat(entities): add updated_at column for entities #10400

Merged
merged 10 commits into from
Mar 15, 2023

Conversation

liverpool8056
Copy link
Contributor

@liverpool8056 liverpool8056 commented Mar 1, 2023

Summary

add updated_at column for entities

Checklist

Full changelog

  • [Implement ...]

Issue reference

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@liverpool8056 liverpool8056 force-pushed the feat/add-updated_at-column-for-entities branch from 0295079 to 5b60f5c Compare March 1, 2023 08:34
spec/02-integration/02-cmd/10-migrations_spec.lua Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@liverpool8056 liverpool8056 force-pushed the feat/add-updated_at-column-for-entities branch from 712f503 to 46b8c34 Compare March 1, 2023 09:16
@hbagdi hbagdi added testing Discussions around writing Kong tests and removed testing Discussions around writing Kong tests labels Mar 1, 2023
@hbagdi
Copy link
Member

hbagdi commented Mar 1, 2023

What about workspaces.lua, clustering_data_planes.lua?
Should we not add the missing timestamps in them as well?

@liverpool8056
Copy link
Contributor Author

@hbagdi workspaces is not supported in ce, I will do make it in ee.
For clustering_data_planes, I think it is not an user-oriented entity, and I have no idea in which scenario we would use it, so not added this time.

@liverpool8056 liverpool8056 force-pushed the feat/add-updated_at-column-for-entities branch from 3fd4266 to eafa35f Compare March 2, 2023 05:48
Copy link
Contributor

@chronolaw chronolaw left a comment

Choose a reason for hiding this comment

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

If we need not to deal with version compatibility problem, I think this PR is OK.

@vm-001
Copy link
Contributor

vm-001 commented Mar 2, 2023

@hbagdi workspaces is not supported in ce, I will do make it in ee. For clustering_data_planes, I think it is not an user-oriented entity, and I have no idea in which scenario we would use it, so not added this time.

The workspace schema is under the kong/db/schema/entities folder. I think we can add updated_at to workspace as well.

Copy link
Contributor

@vm-001 vm-001 left a comment

Choose a reason for hiding this comment

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

It looks good overall.

I'm not sure if adding a new field for these core entities would break DP-CP version compatibility.

@hbagdi
Copy link
Member

hbagdi commented Mar 2, 2023

@hbagdi workspaces is not supported in ce, I will do make it in ee.

The entity definition for it is right here: https://github.com/Kong/kong/blob/master/kong/db/schema/entities/workspaces.lua
Please introduce the timestamps in this PR. Adding it only in EE introduces unnecessary conflicts.

For clustering_data_planes, I think it is not an user-oriented entity, and I have no idea in which scenario we would use it, so not added this time.

The entity is user-facing and we expose a read-only endpoint for it: https://github.com/Kong/kong/blob/master/kong/api/routes/clustering.lua#L22.
The data for this entity is generated by the CP when DPs connect.
Please introduce the timestamps as it helps users determine when a DP was first connected to the CP.

@liverpool8056
Copy link
Contributor Author

liverpool8056 commented Mar 3, 2023

@hbagdi The last_seen field in clustering_data_plane seems have the same semantic with updated_at? If so, do we still have to add it?

@chronolaw chronolaw changed the title feat(entities): add updated_at column for entities feat(entities): add updated_at column for entities Mar 3, 2023
@hbagdi
Copy link
Member

hbagdi commented Mar 3, 2023

@hbagdi The last_seen field in clustering_data_plane seems have the same semantic with updated_at? If so, do we still have to add it?

I recommend doing so for consistency than anything else. I see the point that it doesn't provide additional value.
I leave the decision up to you, I'm fine either way.

@hbagdi hbagdi added testing Discussions around writing Kong tests and removed testing Discussions around writing Kong tests labels Mar 6, 2023
@liverpool8056 liverpool8056 force-pushed the feat/add-updated_at-column-for-entities branch from 9718c8b to 584bf2f Compare March 7, 2023 02:39
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.

This one fix is still needed.

kong/db/migrations/core/019_320_to_330.lua Outdated Show resolved Hide resolved
@bungle bungle force-pushed the feat/add-updated_at-column-for-entities branch from f03d4b5 to b758f57 Compare March 15, 2023 12:41
@bungle
Copy link
Member

bungle commented Mar 15, 2023

@liverpool8056 are the migration tests broken (because of something outside this PR)?

@bungle bungle force-pushed the feat/add-updated_at-column-for-entities branch from b758f57 to 86d5299 Compare March 15, 2023 14:40
@bungle bungle merged commit ecbf062 into master Mar 15, 2023
@bungle bungle deleted the feat/add-updated_at-column-for-entities branch March 15, 2023 15:19
@vm-001
Copy link
Contributor

vm-001 commented Mar 16, 2023

After taking another look, there is something that should be adjusted. Have synced with @liverpool8056 offline.

1/ missing has_updated = true after modifying DP config.

config_entity.updated_at = nil

(Should add nil check before setting x.updated = nil

2/ Should add a test case against the new-added compatibility logic in spec/01-unit/19-hybrid/03-compat_spec.lua

liverpool8056 added a commit that referenced this pull request Mar 16, 2023
that forgetting to set the `has_updated` flag
hanshuebner pushed a commit that referenced this pull request Mar 29, 2023
liverpool8056 added a commit that referenced this pull request Apr 27, 2023
liverpool8056 added a commit that referenced this pull request May 18, 2023
windmgc pushed a commit that referenced this pull request May 18, 2023
…n dp and cp (#10759)

This PR is to add a config compatibility test that should have been added in [#10400](#10400)
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.

7 participants