8000 feat: Added new method: check for new unlisted tags by airween · Pull Request #3437 · coreruleset/coreruleset · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

feat: Added new method: check for new unlisted tags #3437

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
Merged
merged 13 commits into from
Jan 9, 2024
Merged

feat: Added new method: check for new unlisted tags #3437

merged 13 commits into from
Jan 9, 2024

Conversation

airween
Copy link
Contributor
@airween airween commented Dec 23, 2023

Motivation: store actually used tags in a file and check PR's that any rules contains a new (unlisted) tag or not.

With this step, we can exclude the addition of unwanted new tags or the misspelling of tags and helps to keep tags in a consistent state.

@airween airween requested review from theseion and fzipi December 23, 2023 19:48
theseion
theseion requested changes Dec 24, 2023
View reviewed changes
airween and others added 7 commits December 24, 2023 10:18
@airween @theseion
Update util/crs-rules-check/README.md
6f819d4 
Co-authored-by: Max Leske <th3s3ion@gmail.com>
@airween @theseion
Update util/crs-rules-check/README.md
4e41063 
Co-authored-by: Max Leske <th3s3ion@gmail.com>
@airween @theseion
Update util/crs-rules-check/rules-check.py
2a4a292 
Co-authored-by: Max Leske <th3s3ion@gmail.com>
@airween @theseion
Update util/crs-rules-check/rules-check.py
74cde8c 
Co-authored-by: Max Leske <th3s3ion@gmail.com>
@airween @theseion
Update util/crs-rules-check/rules-check.py
12e57a1 
Co-authored-by: Max Leske <th3s3ion@gmail.com>
@airween @theseion
Update util/crs-rules-check/rules-check.py
f174d8c 
Co-authored-by: Max Leske <th3s3ion@gmail.com>
theseion
theseion previously approved these changes Dec 24, 2023
View reviewed changes
@airween
Copy link
Contributor Author
airween commented Dec 24, 2023

Please do not merge this until other devs haven't seen. I'm curious about others opinion too.

fzipi
fzipi previously requested changes Dec 28, 2023
View reviewed changes
Copy link
Member
@fzipi fzipi left a comment

Choose a reason for hiding this comment

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

I think we should name it APPROVED_TAGS instead of USED_TAGS.

@airween
Copy link
Contributor Author
airween commented Dec 28, 2023

I think we should name it APPROVED_TAGS instead of USED_TAGS.

Thanks. I wait with the modifications till other opinions.

@theseion
Copy link
Contributor

I like the idea to use APPROVED_TAGS.

@dune73
Copy link
Member
dune73 commented Jan 3, 2024

No strong feelings on the APPROVED_TAGS vs USED_TAGS question.

Maybe this config file should not be on the top level, though. It looks a bit out of place. Maybe in tests or util folder.

I like the supplementary --tags-list option.

Other than that I do like the general idea of the PR.

@airween
Copy link
Contributor Author
airween commented Jan 3, 2024

Maybe this config file should not be on the top level, though. It looks a bit out of place. Maybe in tests or util folder.

@fzipi, @theseion, @dune73 - please help me what would be the good place for the file of list of tags.

@dune73
Copy link
Member
dune73 commented Jan 5, 2024

There are not that many options. Checking the landscape a bit, I think util would be right place. Right next to the file id-range.

@theseion
Copy link
Contributor
theseion commented Jan 5, 2024

Maybe this config file should not be on the top level, though. It looks a bit out of place. Maybe in tests or util folder.

@fzipi, @theseion, @dune73 - please help me what would be the good place for the file of list of tags.

For me there are three options, in order of preference:

  • top-level
  • util/crs-rules-check
  • rules

I wouldn't put it into rules because all the files in rules currently pertain to rule content only, not metadata.
I would rather not put it into util/crs-rules-check because it's core data, not particularly associated with the rules check script.

@airween
Copy link
Contributor Author
airween commented Jan 6, 2024

Thank you all. After thinking about it, may be the top-level isn't a good choice. I wouldn't put it beside the rules-check script, because I'm sure it doesn't belong that - it belongs to CRS itself.

I think the utils directory will be a goo place.

Adding the cli switch --tags-list is a good idea, but then it must be mandatory.

I'm going to rename the file to APPROVED_TAGS.

Note: the idea of this feature came from here.

@airween airween requested a review from fzipi January 7, 2024 19:08
@airween
Copy link
Contributor Author
airween commented Jan 7, 2024

Every requested modifications are done.

@dune73 dune73 mentioned this pull request Jan 8, 2024
Closed
@dune73
Copy link
Member
dune73 commented Jan 9, 2024

Given the APPROVED_TAG renaming was implemented like @fzipi asked for, I'm dismissing his review and merging. Thank you all.

@dune73 dune73 dismissed fzipi’s stale review January 9, 2024 15:29

Has been implemented.

@dune73 dune73 merged commit 51811bb into coreruleset:v4.0/dev Jan 9, 2024
@github-actions github-actions bot mentioned this pull request Jan 10, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@theseion theseion theseion approved these changes

@fzipi fzipi Awaiting requested review from fzipi

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

Successfully merging this pull request may close these issues.
4 participants
@airween @theseion @dune73 @fzipi
0