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

Add support to negated paths definition in the variable containing paths to ignore on policies updates #642

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

disaverio
Copy link

Fixes Issue

Closes #641

Changes proposed

  • If !myFolder/subfolder/** is defined in the variable then all the contents of it should not be ignored, even if the path myFolder/** is defined to be ignored
  • If the only path defined in the variable is a negated one (or more than one), eg !myFolder, then nothing is ignored: the default behavior still is "nothing is ignored, unless here explicitly defined"

Check List (Check all the applicable boxes)

  • I sign off on contributing this submission to open-source
  • My code follows the code style of this project.
  • My change requires changes to the documentation.
  • I have updated the documentation accordingly.
  • All new and existing tests passed.
  • This PR does not contain plagiarized content.
  • The title of my pull request is a short description of the requested changes.

Screenshots

Note to reviewers

Copy link

netlify bot commented Aug 11, 2024

Deploy Preview for opal-docs ready!

Name Link
🔨 Latest commit 5c14092
🔍 Latest deploy log https://app.netlify.com/sites/opal-docs/deploys/673ee83e9216090008a0562d
😎 Deploy Preview https://deploy-preview-642--opal-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@obsd obsd requested a review from roekatz August 12, 2024 12:08
Copy link
Collaborator

@roekatz roekatz left a comment

Choose a reason for hiding this comment

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

Hi @disaverio - looks great! and cool addition.
To be clear - a "path to not ignore" (e.g !somefolder) would always take precedence even if subpaths are explicitly being ignored.

That is I can't go with further iterations of inclusion-exclusion like in "ignore-me/**;!ignore-me/but-not-me;ignore-me/but-not-me/me-as-well"

I'm not saying this is a super important use case to support - but rather that the behavior in that case is not clear without looking at the code.
So I would consider maybe having the paths "not to ignore" on a different config var and make clear it has the last say.
Or if you don't like that suggestion - just add a short comment about it in the existing docs.

Thanks!

@disaverio
Copy link
Author

Hi @disaverio - looks great! and cool addition. To be clear - a "path to not ignore" (e.g !somefolder) would always take precedence even if subpaths are explicitly being ignored.

Hi @roekatz, thanks for your review.

Yes, I confirm: a "path to not ignore" would always take precedence. It is also made explicit in the two tests i added: test_should_ignore_path_keeping_higher_priority_to_ones_defined_as_not_to_ignore_C and test_should_ignore_path_keeping_higher_priority_to_ones_defined_as_not_to_ignore_D.

I'm not saying this is a super important use case to support - but rather that the behavior in that case is not clear without looking at the code. So I would consider maybe having the paths "not to ignore" on a different config var and make clear it has the last say. Or if you don't like that suggestion - just add a short comment about it in the existing docs.

I will make it more clear by adding a more explicit, punctual description in the documentation. Still, if you prefer to use a dedicated config var, pls do tell me :)

Thanks!

Copy link
Collaborator

@roekatz roekatz left a comment

Choose a reason for hiding this comment

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

Amazing!
Will merge once I stabilize master tests :)

@roekatz roekatz self-requested a review August 28, 2024 12:14
Copy link
Collaborator

@roekatz roekatz left a comment

Choose a reason for hiding this comment

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

Oh sorry - if you can just fix the pre-commit errors. (You can run it locally, install pre-commit (brew install pre-commit on Mac). Then within opal repo:

pre-commit install
pre-commit run --all-files

@disaverio
Copy link
Author

files have been reformatted ✅

@roekatz
Copy link
Collaborator

roekatz commented Aug 29, 2024

@disaverio Great - Tests should now be fixed in master so if you can rebase we'll see we're green and have it merged.

@disaverio disaverio force-pushed the issue-641-enhanced-ignored-paths-variable branch from 9fe5880 to 605841f Compare September 9, 2024 12:28
@disaverio
Copy link
Author

Hello @roekatz , the branch has been rebased. ty

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.

Enhanced POLICY_STORE_POLICY_PATHS_TO_IGNORE
2 participants