Skip to content

Conversation

@jdelic
Copy link
Contributor

@jdelic jdelic commented Oct 11, 2024

What does this PR do?

Adds MAINTAIN (m) to the supported privileges in salt.modules.postgres.

What issues does this PR fix or reference?

Fixes #66962

Merge requirements satisfied?

[NOTICE] Bug fixes or features added to Salt require tests.

Commits signed with GPG?

Yes

@jdelic jdelic requested a review from a team as a code owner October 11, 2024 20:45
@salt-project-bot-prod-environment salt-project-bot-prod-environment bot changed the title Fix/postgres maintain privilege [3007.x] Fix/postgres maintain privilege Oct 11, 2024
@jdelic jdelic changed the title [3007.x] Fix/postgres maintain privilege [3007.x] Add MAINTAIN (m) privilege to postgres module lookup tables Oct 11, 2024
Copy link
Contributor

@twangboy twangboy left a comment

Choose a reason for hiding this comment

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

Would you mind writing a test for this?

@elpavel
Copy link

elpavel commented Oct 28, 2024

Is there a way to test this @twangboy ? The current test suite doesn't run tests against a Postgres database (if I remember well).

I did implement the same change as in this patch in our salt, to allow privileges management on our Postgres 17 databases. Thanks @jdelic it works OK.

@twangboy
Copy link
Contributor

Looks like there's a test failure.

@jdelic jdelic force-pushed the fix/postgres-maintain-privilege branch from b1aa982 to 613e760 Compare October 31, 2024 21:41
@jdelic
Copy link
Contributor Author

jdelic commented Nov 1, 2024

@twangboy thank you for the approval. I don't think the CI has run my modified tests yet... Any idea how to get them to run? It looks to me like a failure on the test infrastructure 🤷‍♂️

@twangboy
Copy link
Contributor

twangboy commented Nov 6, 2024

With the recent changes to our internal infrastructure mandated echelons above us with minimal notice, most of our pipeline is broken. Once it is fixed you can rebase and the tests should run.

@jdelic
Copy link
Contributor Author

jdelic commented Nov 6, 2024

@twangboy yeah, I realized that the pipelines don't yet point to the Broadcom endpoints. 😁

I'll wait for the 3007.x branch to be fixed and proceed accordingly. Thank you 🙏

@evkuzin
Copy link

evkuzin commented Dec 18, 2024

What are the blockers here? I’m very much in need of this change 🥺

@jdelic
Copy link
Contributor Author

jdelic commented Dec 18, 2024

@evkuzin
like most other new PRs this one is blocked on the URL changes from #67024 and possibly #67022 and #67101 landing. Once these are merged the build infrastructure should work again, at which point I can rerun the required tests, which will allow this one to be merged by the maintainers.

I don't know why the breaking change had to be done in this way instead of the many ways it could have been done that wouldn't have broken users and build systems and support processes, but your guess is as good as mine.

@jdelic
Copy link
Contributor Author

jdelic commented Feb 20, 2025

@twangboy I reran the tests now that #67024 has landed and this is ready to merge 🎉

@dwoz dwoz force-pushed the fix/postgres-maintain-privilege branch from 6d041a0 to cb26d8d Compare May 3, 2025 03:44
@dwoz dwoz added the test:full Run the full test suite label May 3, 2025
@dwoz dwoz merged commit 90e772d into saltstack:3007.x May 3, 2025
723 of 737 checks passed
jdelic added a commit to jdelic/saltshaker that referenced this pull request Aug 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

test:full Run the full test suite

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants