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.

Sign up for GitHub

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

Jump to bottom

Upgrades to CRS v4.0 #11

Merged
merged 2 commits into from
Mar 7, 2024
Merged

Upgrades to CRS v4.0 #11

merged 2 commits into from
Mar 7, 2024

Conversation

M4tteoP
Copy link
Member

@M4tteoP M4tteoP commented Feb 21, 2024 •
edited
Loading

As proposed in the issue, the new tag will follow CRS versioning with a coraza suffix for eventually further changes. Therefore it is going to be v4.0.0-coraza.1.

Closes #10

@jcchavezs
Copy link
Member

jcchavezs commented Feb 21, 2024 via email

@M4tteoP
Copy link
Member Author

M4tteoP commented Feb 21, 2024

We might even just stick with v4.0.0 and only if we actually ever need further changes add the coraza suffix. Adding changes here feels weird to me, I would try to stick to just mirroring the CRS as much as possible

@M4tteoP M4tteoP marked this pull request as ready for review February 21, 2024 10:28
@anuraaga
Copy link
Contributor

anuraaga commented Feb 21, 2024 •
edited
Loading

If I'm not mistaken, if using v4 this will need to go into a folder called v4, or a branch called v4.

Just wondering what is the experience like upgrading from CRS v3 to v4 for example for modsecurity users? In Go, when a library changes major, the import statements have to change and I find it pretty annoying, in the case of this library, there would never be an API change to warrant it. But if the behavior change is too much, then it's probably worth it. The alternative is 1.4.x, 1.5.x

I'm not sure how go would handle the coraza suffix, I have an impression that go interprets suffixes as prereleases or similar. I would just reserve the patch version as space to deviate from upstream, if upstream patch goes up then it goes up here, if something here causes a bump, the patch goes up and doesn't match upstream until the next minor. In practice it shouldn't be a huge issue to not match perfectly.

@jcchavezs
Copy link
Member

Thanks for the input @anuraaga. I like the idea of having an own patch version, we just need to make sure CRS follows semver (opened an issue in coreruleset/coreruleset#3582). Still I think for 4.0.0 we can happily release v4.0.0 and very likely we need to change the module name to v4?

@jcchavezs
Copy link
Member

I am merging this unless further concerns cc @anuraaga

@jcchavezs
Copy link
Member

Friendly ping @anuraaga

anuraaga
anuraaga approved these changes Feb 28, 2024
View reviewed changes
Copy link
Contributor

@anuraaga anuraaga left a comment

Choose a reason for hiding this comment

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

Make sure the module name works properly before tagging since it can be easy to require redactions when using non-v1

@jcchavezs
Copy link
Member

jcchavezs commented Feb 29, 2024 via email

@M4tteoP
Copy link
Member Author

M4tteoP commented Mar 6, 2024

I updated the module to github.com/corazawaf/coraza-coreruleset/v4, I would merge this and then try to make the example point to it.

@jcchavezs jcchavezs merged commit 87d0821 into corazawaf:main Mar 7, 2024
2 checks passed
@jcchavezs jcchavezs mentioned this pull request Mar 9, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jcchavezs jcchavezs jcchavezs approved these changes

@anuraaga anuraaga anuraaga approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Upgrades to CRS 4.0
3 participants
@M4tteoP @jcchavezs @anuraaga