Skip to content

Allow to set IP address to analyze#60

Closed
marcstern wants to merge 105 commits intomaxmind:masterfrom
marcstern:master
Closed

Allow to set IP address to analyze#60
marcstern wants to merge 105 commits intomaxmind:masterfrom
marcstern:master

Conversation

@marcstern
Copy link

Added mod_security2 build, install and config.
Added test to trigger it from the URL (query string).

@marcstern
Copy link
Author

My test install works, but the general Apache setup (from the trunk) is incorrect.
I tried several things but other problems are continuously coming.
By fixing the trunk, my tests should be OK.

@nchelluri
Copy link
Contributor

Hi; I'll try taking a look at this tomorrow. In the mean-time, I'm wondering what the test failures are about, but haven't had a chance to look yet.

@marcstern
Copy link
Author

I fixed 5 problems.
Now, I'm not sure what to use for ServerRoot & ServerName.
Did you copy the test config from another project? I can have a look at it

@nchelluri
Copy link
Contributor

Hi; I'm not sure where the test config came from. I thought I'd have time to take a look at this today but I am a bit backed up with other work. I will look at it tomorrow with a coworker and will get back to you with, at the least, preliminary feedback by the end of the day then.

@nchelluri
Copy link
Contributor

@marcstern I'm really sorry that I haven't found the time to dig into this yet. I promise you, I'll do my best to leave some concrete feedback by the end of the week.

@nchelluri
Copy link
Contributor

Hi @marcstern , both a coworker and I spent several hours trying to get this PR passing tests and failed.

We think it's not a bad idea in principle but it's going to be hard to maintain this patch without tests and given that it's just so hard to get the environment variable set at just the right time (and to document with an example) we prefer not to merge this PR as is.

Thanks for your contribution. If you or someone can come up with a more easily testable and documentable solution, we may consider it, so we're leaving tihs PR open.

@nchelluri
Copy link
Contributor

Closing this PR, updated code is in #66

@nchelluri nchelluri closed this Sep 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants