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

Fix "--disable-flagging-jndi-lookup" flag #58

Merged
merged 3 commits into from
Jan 6, 2022

Conversation

nmiyake
Copy link
Contributor

@nmiyake nmiyake commented Jan 6, 2022

Flag was not properly hooked up, so specifying it had no effect.
Fixes the issue so that specifying the "--disable-flagging-jndi-lookup"
flag causes the crawl operation to properly ignore flagging issues
that are caused only by the presence of the JndiLookup class.

Flag was not properly hooked up, so specifying it had no effect.
Fixes the issue so that specifying the "--disable-flagging-jndi-lookup"
flag causes the crawl operation to properly ignore flagging issues
that are caused only by the presence of the JndiLookup class.
@nmiyake nmiyake requested a review from bmoylan January 6, 2022 21:40
@changelog-app
Copy link

changelog-app bot commented Jan 6, 2022

Generate changelog in changelog/@unreleased

Type

  • Feature
  • Improvement
  • Fix
  • Break
  • Deprecation
  • Manual task
  • Migration

Description

Fix "--disable-flagging-jndi-lookup" flag such that specifying the flag properly ignores files that were flagged only because of the presence of the JndiLookup class.

Check the box to generate changelog(s)

  • Generate changelog entry

Copy link
Contributor

@bmoylan bmoylan left a comment

Choose a reason for hiding this comment

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

It'd be nice if there is a simple test to confirm this, but we don't have to block on it.

@nmiyake
Copy link
Contributor Author

nmiyake commented Jan 6, 2022

Agreed -- in the ideal world we should have unit tests that test all the functionality and then integration tests that test all of the flags.

After starting to make that an excuse to push off adding the integration test, realized it's not a big deal to do so and might as well start now rather than requiring all or nothing. In doing so, uncovered an edge case, so there we go!

Copy link
Contributor

@bmoylan bmoylan left a comment

Choose a reason for hiding this comment

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

awesome, thanks!

@bulldozer-bot bulldozer-bot bot merged commit 918271e into develop Jan 6, 2022
@bulldozer-bot bulldozer-bot bot deleted the fixDisableJndiLookup branch January 6, 2022 22:08
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.

None yet

3 participants