Skip to content
This repository has been archived by the owner on May 14, 2020. It is now read-only.

Refactoring ModSecurity CRS Branch v3.3/dev #1627

Merged
merged 1 commit into from
Dec 2, 2019
Merged

Refactoring ModSecurity CRS Branch v3.3/dev #1627

merged 1 commit into from
Dec 2, 2019

Conversation

soufianebenali
Copy link
Contributor

These changes implement part 1 of the agreed refactoring explained in #1600:

  • Create a new folder tests in the root folder
  • Move util/regression-tests/ -> tests/regression/, and util/integration/ -> tests/integration
  • Rename folder documentation/ to docs/
  • Create folder examples/ and move crs-setup.conf.example -> examples/crs-setup.conf

@soufianebenali soufianebenali changed the title Refactoring ModSecurity CRS Refactoring ModSecurity CRS Branch v3.3/dev Nov 19, 2019
@bittner
Copy link
Contributor

bittner commented Nov 20, 2019

The build is failing (for this and all related PRs) currently, because we move and rename the crs-setup.conf.example file, which in turn is used in the Dockerfile cloning the current repository.

This problem would go away automatically once this PR were merged and/or the Dockerfile moved out to the separate repository that we have agreed on.

To make the build pass here, however, we would have to undo the renaming and get the PRs all merged first, and then do the moving / renaming of crs-setup.conf.example again in an additional PR (for each branch). We're about to prepare this as I write these lines.

Affected PRs

@fgsch
Copy link
Contributor

fgsch commented Dec 1, 2019

From a cursory check this looks OK to me.

@bittner
Copy link
Contributor

bittner commented Dec 2, 2019

Who could merge this, so we can continue? As explained earlier, there are still a few changes remaining, which depend on these changes to be merged first. It would help if we could move fast. 🙏

@dune73
Copy link
Contributor

dune73 commented Dec 2, 2019

Started to look into this. I see this viable for 3.3, but there are governance issues with 3.1 and 3.2. We probably did not think of this enough, but changing the directory structure of existing stable release lines is a project governance problem.

A casual look brought up two issues for me:

  • I see a change: documentation/README → docs/documentation/README
    Is this really wanted? Where is docs/README?
  • Why is wording not adjusted inside the documentation with this change? (-> INSTALL file, documentation/README, etc.)

Tonight is another CRS community chat. Is one of you guys attending (20:30 CET)? We really tried to cover your plans last month, but with you missing and @csanders-git not being there for large parts of the meeting, we could not get to the bottom of it.

I would really like to get this over to move to other topics again. Ideally we can sort out most of it tonight and merge during the meeting or afterwards. But it is likely we need a few little changes to the PR first. So better be around.

@dune73
Copy link
Contributor

dune73 commented Dec 2, 2019

Thank you.

@dune73 dune73 merged commit 48dfe80 into SpiderLabs:v3.3/dev Dec 2, 2019
@bittner bittner deleted the refactor/v3.3/dev branch December 3, 2019 06:25
@bittner
Copy link
Contributor

bittner commented Dec 3, 2019

Thanks for merging this PR! 🥇

If the decision for the 3.2 and 3.1 branches is definite please close the related PRs with an appropriate comment. Thank you!

That does mean that we should do all the refactoring, also the automated builds of the Docker images, only for 3.3+, and leave older branches as is. Can you confirm?

@bittner
Copy link
Contributor

bittner commented Dec 3, 2019

  • There is a single file that has remained in util/regression-tests/tests/. Should we also move this to /tests/regression/ or is there a reason for leaving it in the old location?
  • util/README still mentions the regression tests location in util/regression-tests. We will clean this up.

Some additional thoughts:

  • The content in util/crs2-renumbering is probably there for historical (and/or sentimental) reasons. You may want to clean this up one day. Should we do this on your behalf?
  • Some other utilities (scripts, configuration files) seem to be related either to support manual verification or similar test activities. Maybe it makes sense to gather them also somewhere in the /tests folder?

@dune73
Copy link
Contributor

dune73 commented Dec 4, 2019

Future docker PRs only for 3.3: It depends a bit, but that's probably the case. I suggest you issue them against 3.3 and the project can then decide on backporting. Feel free to suggest that in the PR. In the case we want to backport, it is likely, the project would get back to you and ask for your support.

921190.yaml: Yes, please move this too. Probably a new YAML that was not yet covered in your PR.

Thanks for looking into util/README. I had this on my radar too.

Please leave crs2-renumbering as it is. We had removed it in 3.1, but brought it back for 3.2 on popular request.

Other scripts: It really depends and I do not have an exact overview.

@bittner
Copy link
Contributor

bittner commented Dec 4, 2019

I opened PR #1646 to apply the mentioned changes. Thank you!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants