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

#243: Rename pgtde_is_encrypted() to pg_tde_is_encrypted() #242

Merged
merged 4 commits into from
Jul 23, 2024

Conversation

jeffreydwalter
Copy link
Contributor

@jeffreydwalter jeffreydwalter commented Jul 17, 2024

See: #243

@jeffreydwalter jeffreydwalter changed the title Fix is_encrypted function name prefix https://github.com/Percona-Lab/pg_tde/issues/243 Jul 17, 2024
@jeffreydwalter jeffreydwalter changed the title https://github.com/Percona-Lab/pg_tde/issues/243 #243: Rename pgtde_is_encrypted() to pg_tde_is_encrypted() Jul 17, 2024
Copy link
Member

@dAdAbird dAdAbird left a comment

Choose a reason for hiding this comment

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

Hey @jeffreydwalter,
the expected/pgtde_is_encrypted.out file has to be renamed as well

Thanks for the PR

@jeffreydwalter jeffreydwalter requested a review from dAdAbird July 21, 2024 05:19
@jeffreydwalter
Copy link
Contributor Author

Happy to help!

Copy link
Collaborator

@ImTheKai ImTheKai left a comment

Choose a reason for hiding this comment

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

LGTM

@dAdAbird
Copy link
Member

It needs a couple of more changes in tests' expected files. See the regression.diff here: https://github.com/Percona-Lab/pg_tde/actions/runs/10025861865/artifacts/1725344792

@jeffreydwalter
Copy link
Contributor Author

jeffreydwalter commented Jul 22, 2024

It needs a couple of more changes in tests' expected files. See the regression.diff here: https://github.com/Percona-Lab/pg_tde/actions/runs/10025861865/artifacts/1725344792

@dAdAbird can you please tell me what changes need to be made? It's not really obvious to me, from the diff, what the problem is. It looks like it might be a GitHub action cache issue.

@dutow
Copy link
Collaborator

dutow commented Jul 22, 2024

@jeffreydwalter The dashed lines need one more dash because now the function name is one character longer. It's more visible if you add a space manually to the diff - without that, it's difficult to see because the diff also uses the - as a marker:

 SELECT pg_tde_is_encrypted('test_norm');
  pg_tde_is_encrypted 
-  --------------------
+  ---------------------

You can also execute the tests locally and use the test runner to generate the expected files, that prevents issues like this.

@jeffreydwalter
Copy link
Contributor Author

@jeffreydwalter The dashed lines need one more dash because now the function name is one character longer. It's more visible if you add a space manually to the diff - without that, it's difficult to see because the diff also uses the - as a marker:

 SELECT pg_tde_is_encrypted('test_norm');
  pg_tde_is_encrypted 
-  --------------------
+  ---------------------

You can also execute the tests locally and use the test runner to generate the expected files, that prevents issues like this.

Ah, I opened the diff in a text editor, so it wasn't obvious because the - blended into the line. 😆
image

Anyway, I think I get why you guys do testing that way, but surely there's got to be a better way... Thanks for your help!

@jeffreydwalter jeffreydwalter requested a review from ImTheKai July 23, 2024 03:01
@dAdAbird
Copy link
Member

Yeah, those alignment things might be quite annoying. But renaming or similar changes concerning existing tests shouldn't happen much...
Anyway, thanks for your contribution

@ImTheKai ImTheKai merged commit 80ae7d8 into percona:main Jul 23, 2024
10 checks passed
ImTheKai pushed a commit to ImTheKai/pg_tde that referenced this pull request Aug 7, 2024
…rcona#242)

* Fix is_encrypted function name prefix

* main Rename file

* Increase dashed line length to match psql output
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.

4 participants