Skip to content

Conversation

@brunocascio
Copy link
Contributor

Fixes #288

Motivation

When roles get removed from PulsarPermission's roles property, it must be removed from Pulsar permissions.

Modifications

  • Added 2 methods (GetTopicPermissions and GetNamespacePermissions)
  • Modified the reconcile permission logic, in order to grant or revoke permissions based on the incoming roles.
  • Integrations tests.
  • Updated conributing guidelines
  • Replaces some hardcoded strings with env variables (backward compatible)

Verifying this change

  • Make sure that the change passes the CI checks.

This change added tests and can be verified as follows:

  • Added integration tests for PulsarPermission

Documentation

Check the box below.

Need to update docs?

  • no-need-doc

Because it's a fix

@brunocascio brunocascio requested review from a team as code owners March 11, 2025 18:17
@github-actions github-actions bot added the no-need-doc This pr does not need any document label Mar 11, 2025
@brunocascio
Copy link
Contributor Author

Hello folks,

How should I proceed with this PR in order to get merged?

Thanks

@lhotari
Copy link
Member

lhotari commented Apr 8, 2025

Hello folks,

How should I proceed with this PR in order to get merged?

Thanks

Thanks for the contribution @brunocascio. Great work! I'll help getting this merged.

@lhotari
Copy link
Member

lhotari commented Apr 8, 2025

@freeznet Please review

Copy link
Member

@lhotari lhotari left a comment

Choose a reason for hiding this comment

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

LGTM, good work @brunocascio

@lhotari lhotari merged commit 40abfa1 into streamnative:main Apr 16, 2025
5 checks passed
@lhotari
Copy link
Member

lhotari commented Jun 3, 2025

It seems that this change causes a regression #310 . @brunocascio do you have a chance to take a look?

@brunocascio
Copy link
Contributor Author

It seems that this change causes a regression #310 . @brunocascio do you have a chance to take a look?

maybe it's better to rollback this change due to it breaks currenty functionality of permissions.
Unfortunately I don't have the time now, but definetly I will look into it asap.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no-need-doc This pr does not need any document

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Operator does not revoke roles when removed from the PulsarPermission's CR

3 participants