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 string escaping #2

Merged
merged 3 commits into from
Feb 20, 2024
Merged

Conversation

isinyaaa
Copy link

Description

I started by adding a gitignore as a basic chore to help me debug this, as well.

Fixed a string escape bug that was preventing us from adding certain values on the table using PSQL.
The behavior should remain unaltered for other DB backends.

I've also bumped the PSQL-12 client version.

How Has This Been Tested?

Tested with the Python MLMD client.

Merge criteria:

  • The commits are squashed in a cohesive manner and have meaningful messages.
  • Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
  • The developer has manually tested the changes and verified that the changes work

@isinyaaa isinyaaa self-assigned this Feb 15, 2024
Copy link
Member

@tarilabs tarilabs left a comment

Choose a reason for hiding this comment

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

I have not experience with cpp but I reckon this introduces proper string escaping.

I also notice version bumps in bazel will likely require a different set of "snapshotted dependency cache" for which @rimolive @lampajr are more familiar.

Thank you!

@tarilabs tarilabs requested a review from lampajr February 16, 2024 07:35
@rareddy
Copy link

rareddy commented Feb 16, 2024

Any tests to verify you can add to the PR?

@isinyaaa
Copy link
Author

You can create a new metadata store pointing to a Postgres DB, and then experiment with text you would usually insert for e.g. a description formatted as markdown (which is what motivated this patch).

Copy link

@lampajr lampajr left a comment

Choose a reason for hiding this comment

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

I also notice version bumps in bazel will likely require a different set of "snapshotted dependency cache" for which @rimolive @lampajr are more familiar.

+1 once we will create the release branch we should have to re-generate the external dependencies/cache archive

The change lgtm but I'd like to test it together with #1 in order to be sure that the build would work on ubi/fedora images

lampajr added a commit that referenced this pull request Feb 19, 2024
* Fix MLMD build on Fedora

Signed-off-by: Andrea Lamparelli <[email protected]>

* Revert zetasql upgrade and switch to ubi8 (#2)

---------

Signed-off-by: Andrea Lamparelli <[email protected]>
@lampajr
Copy link

lampajr commented Feb 19, 2024

I'd hold this PR a little bit because I'd like to setup a simple CI that performs a container build of the Dockerfile.redhat I added with #1 - this way we can check if the UBI-based build still work with these changes

@lampajr
Copy link

lampajr commented Feb 19, 2024

I'd hold this PR a little bit because I'd like to setup a simple CI that performs a container build of the Dockerfile.redhat I added with #1 - this way we can check if the UBI-based build still work with these changes

Created #3 to the simple ci, this will ensure that a ubi-based image will work.

Signed-off-by: Isabella Basso do Amaral <[email protected]>
Signed-off-by: Isabella Basso do Amaral <[email protected]>
Signed-off-by: Isabella Basso do Amaral <[email protected]>
Copy link

@dhirajsb dhirajsb left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link

@rimolive rimolive left a comment

Choose a reason for hiding this comment

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

/lgtm

I did not test the changes, but as there's no test instructions and according to Andrea's comment, you will provide an automated way to test PRs.

@isinyaaa isinyaaa merged commit a77d72a into opendatahub-io:master Feb 20, 2024
1 check passed
@isinyaaa isinyaaa deleted the fix-escape-str branch February 20, 2024 20:12
@lampajr lampajr mentioned this pull request Feb 21, 2024
3 tasks
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.

6 participants