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

Reorganized repository structure #77

Merged
merged 5 commits into from
Jan 25, 2023
Merged

Reorganized repository structure #77

merged 5 commits into from
Jan 25, 2023

Conversation

keithf4
Copy link
Collaborator

@keithf4 keithf4 commented Dec 12, 2022

Reorganized structure of repository to make managing of files easier for the build process. Adapted from methods used in https://github.com/pgpartman/pg_partman.

Updates moved into updates folder
C code moved into src folder
SQL code moved into sql folder
Moved test sql script into test folder and renamed for clarity compared to the extension core code file

Removed testing for versions of PostgreSQL no longer in community support

@keithf4 keithf4 requested a review from mpalmi December 12, 2022 18:46
@keithf4 keithf4 self-assigned this Dec 12, 2022
@keithf4 keithf4 requested a review from pgguru December 12, 2022 18:49
@keithf4
Copy link
Collaborator Author

keithf4 commented Dec 12, 2022

@mpalmi Trying to do some reorg on this to make it a little easier to maintain going forward. I did move the C files into their own directory like I have on pg_partman and running make install seems to work ok for me using PGXS but doesn't seem to be working in the github testing. Will try and mess around and see if I can figure it out, but any assistance would be great. Thanks!

@keithf4
Copy link
Collaborator Author

keithf4 commented Dec 12, 2022

I got 12 - 15 passing now. Learned all about pg_regress!

Looks like 11 is still failing because that is no longer available from where this is pulling PG from for the images?

Copy link
Collaborator

@pgguru pgguru left a comment

Choose a reason for hiding this comment

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

So just to confirm, this is failing in the same way as the pre-reorg (i.e., PG 11 only). Do we just want to remove the pgver for 11 in the matrix, or is coverage there important enough to figure things out (and presumably get a new Docker image as a base)?

@keithf4
Copy link
Collaborator Author

keithf4 commented Dec 20, 2022

So just to confirm, this is failing in the same way as the pre-reorg (i.e., PG 11 only). Do we just want to remove the pgver for 11 in the matrix, or is coverage there important enough to figure things out (and presumably get a new Docker image as a base)?

Yes, it's failing in the same way it currently fails with the released version. I don't see anything in the code that's specific to 11, so it's not lacking in a code coverage sense if we excluded it. Importance of coverage there I think would really fall on the team that's primarily doing the development work on this extension.

@keithf4
Copy link
Collaborator Author

keithf4 commented Dec 29, 2022

So just to confirm, this is failing in the same way as the pre-reorg (i.e., PG 11 only). Do we just want to remove the pgver for 11 in the matrix, or is coverage there important enough to figure things out (and presumably get a new Docker image as a base)?

Yes, it's failing in the same way it currently fails with the released version. I don't see anything in the code that's specific to 11, so it's not lacking in a code coverage sense if we excluded it. Importance of coverage there I think would really fall on the team that's primarily doing the development work on this extension.

Looked into this a bit more. This is using the github hosted runners for the regression test workflows. This only has Ubuntu as a target Linux OS (was going to try switching to something RHEL based if possible).

https://docs.github.com/en/actions/using-jobs/choosing-the-runner-for-a-job#choosing-github-hosted-runners

Looks like if we really need to keep PG11 as part of this testing, we'd have to switch to a self-hosted runner since the ones that Github provides are what-you-see-is-what-you-get.

Removed PG11 testing for now to see how the result would look and it's all green now. Let me know if there's another way you'd like to move forward with this.

@keithf4 keithf4 requested a review from pgguru December 29, 2022 14:42
@pgguru
Copy link
Collaborator

pgguru commented Dec 29, 2022

So just to confirm, this is failing in the same way as the pre-reorg (i.e., PG 11 only). Do we just want to remove the pgver for 11 in the matrix, or is coverage there important enough to figure things out (and presumably get a new Docker image as a base)?

Yes, it's failing in the same way it currently fails with the released version. I don't see anything in the code that's specific to 11, so it's not lacking in a code coverage sense if we excluded it. Importance of coverage there I think would really fall on the team that's primarily doing the development work on this extension.

Looked into this a bit more. This is using the github hosted runners for the regression test workflows. This only has Ubuntu as a target Linux OS (was going to try switching to something RHEL based if possible).

https://docs.github.com/en/actions/using-jobs/choosing-the-runner-for-a-job#choosing-github-hosted-runners

Looks like if we really need to keep PG11 as part of this testing, we'd have to switch to a self-hosted runner since the ones that Github provides are what-you-see-is-what-you-get.

Removed PG11 testing for now to see how the result would look and it's all green now. Let me know if there's another way you'd like to move forward with this.

Assuming the pg 11 is only test coverage and not support, I’m personally fine with keeping the GH coverage unless there are reasons/approval/infra to setup our own runner.

@keithf4 keithf4 merged commit 35e9f3f into pgaudit:master Jan 25, 2023
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.

2 participants