Skip to content

Indentation check CI and helper scripts #32

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

Merged
merged 2 commits into from
Jan 27, 2025

Conversation

dutow
Copy link
Collaborator

@dutow dutow commented Jan 20, 2025

This pull request adds indnetation checks on a github action, and helper scripts which can be used to run pgindent easily locally.

There are two scripts:

  • dump-typedefs.sh will create a new typedef file which includes Percona specific types both in postgres itself and pg_tde
  • run-pgindent runs pgindent using this typedef file

The entire source tree is also reformatted using this script.

Since my other PR also contains source code changes, this PR is based on that. Only the last 2 commits are actually part of this PR, please only review those as part of this.

@dutow dutow changed the title Indentation check CI an helper scripts Indentation check CI and helper scripts Jan 20, 2025
@dutow dutow force-pushed the indentfix branch 6 times, most recently from 281ad6c to 20ac5fd Compare January 20, 2025 22:28
Copy link
Collaborator

@jeltz jeltz left a comment

Choose a reason for hiding this comment

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

I feel this MR should be split in two, the third commit does not really belong unless I misunderstand things, plus that the first commit should be a pure re-indent run and not contain other things like it does now.

Also ssaw that some of your files does not end in newline.



EXTRA_REGRESS_OPTS="--extra-setup=$SCRIPT_DIR/tde_setup.sql --load-extension=pg_tde" make installcheck-world -k
Copy link
Collaborator

Choose a reason for hiding this comment

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

This file should end with a newline.

contrib/pg_tde/src16/.*
contrib/pg_tde/src17.*
contrib/pg_tde/src/libkmip/.*
src/backend/nodes/nodetags.h
Copy link
Collaborator

Choose a reason for hiding this comment

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

This file should end with a newline.

./configure --enable-debug --enable-cassert --enable-tap-tests --prefix=$INSTALL_DIR
make install-world
Copy link
Collaborator

Choose a reason for hiding this comment

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

Missing newline at end of file.


- name: Run pgindent
run: src/.scripts/run-pgindent.sh --check --diff

Copy link
Collaborator

Choose a reason for hiding this comment

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

Missing newline at end of file.

@dutow
Copy link
Collaborator Author

dutow commented Jan 21, 2025

@jeltz The first commit is #31 I based on this PR on that because it also contain code changes. I mentioned this in the PR description - the PR is only about the other 2 commits.

@jeltz
Copy link
Collaborator

jeltz commented Jan 21, 2025

@jeltz The first commit is #31 I based on this PR on that because it also contain code changes. I mentioned this in the PR description - the PR is only about the other 2 commits.

Oh, sorry, missed that part of the PR description.

Copy link
Collaborator

@jeltz jeltz left a comment

Choose a reason for hiding this comment

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

Other than cleaning up the commits so the right changes are done in the right commit this looks good. :)

}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this file changed in this commit? It should probably be changed in its own commit.

}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this file changed in this commit? It should probably be changed in its own commit.

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.

A small suggestion but otherwise LGTM

Comment on lines +67 to +68
contrib/pg_tde/src16/.*
contrib/pg_tde/src17.*
Copy link
Member

Choose a reason for hiding this comment

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

does it make sense do have something like

contrib/pg_tde/src1.*

or

contrib/pg_tde/src[0-9].*

so no need to update it with PG v18 and so on

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, maybe, but this is temporary as we plan to remove these completely after the next release, so it's only for a few weeks.

@dutow dutow force-pushed the indentfix branch 3 times, most recently from ec4b664 to 33b4c9e Compare January 22, 2025 18:35
@dutow dutow merged commit b9af932 into percona:TDE_REL_17_STABLE Jan 27, 2025
13 checks passed
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