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

Prepare to migrate to the new versioning system #65

Merged
merged 10 commits into from
Feb 1, 2023

Conversation

Digitalone1
Copy link

@Digitalone1 Digitalone1 commented Jan 21, 2023

Requirements

  • Filling out the template is required.

  • All new code requires tests to ensure against regressions.

    • However, if your PR contains zero code changes, feel free to select the checkmark below to indicate so.
  • Have you ran tests against this code?

Description of the Change

This is the first part of #64 and contains #62. I tried to introduce a view to help in sorting the versions but unfortunately it doesn't work because the order is lost while joining, so we are forced to write the ORDER BY clause every time we do a query.

  • Reverted part of the changes in Improve versions management #42
  • Added created/updated timestamps to versions table, plus the update trigger
  • New code refactoring using the new ORDER BY clause
  • Started to ignore the latest status (which will be dropped in the future) and selecting the latest version relying on the sorting method
  • Introduce the DISTINCT ON PostgreSQL extension to select the latest versions and make use of Common Table Expressions
  • Fixed the existing sorting method introducing a new getOrderField internal function to retrieve the proper column
  • Added new tests for the sorting system (moved the deletion tests at the end)

@confused-Techie
Copy link
Member

Thanks a ton for this! I'm reviewing the code now and just to confirm, that if this is merged we are good to push our backlog of changes to prod (With of course the needed db changes) correct?

But again thanks for the contribution!

@Digitalone1
Copy link
Author

Yes, but maybe it's better to wait for the other changes in #64

@confused-Techie
Copy link
Member

Yes, but maybe it's better to wait for the other changes in #64

Copy that! Sounds like a plan

Copy link
Member

@confused-Techie confused-Techie left a comment

Choose a reason for hiding this comment

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

Overall these changes look good. I'll want to dig a little deeper during testing. but at a readonly review this is looking great so far, and looks like it's changing exactly what has been discussed previously.

Like the cleaning up of our tests, and bundling the Names Supply Chain Attack fixes in here.
Thanks for all the effort!

@confused-Techie confused-Techie mentioned this pull request Jan 25, 2023
2 tasks
@confused-Techie confused-Techie merged commit 5b0bdf1 into pulsar-edit:main Feb 1, 2023
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

Successfully merging this pull request may close these issues.

2 participants