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

Migrated the Ruby Linter to NodeJS Linter #16316

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

Ajay-singh1
Copy link

@Ajay-singh1 Ajay-singh1 commented Mar 12, 2025

Description

This PR replaces the Ruby linter to a modern NodeJS linter which is more flexible and advanced.

The linter passes all the docs test successfully and also it resolves #7906.

This PR fixes issue No. #15148

Reviewers

  • Ambient
  • Docs
  • Installation
  • Networking
  • Performance and Scalability
  • Extensions and Telemetry
  • Security
  • Test and Release
  • User Experience
  • Developer Infrastructure
  • Localization/Translation

@Ajay-singh1 Ajay-singh1 requested a review from a team as a code owner March 12, 2025 06:22
@istio-testing istio-testing added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. needs-ok-to-test labels Mar 12, 2025
@istio-testing
Copy link
Contributor

Hi @Ajay-singh1. Thanks for your PR.

I'm waiting for a istio member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@Ajay-singh1
Copy link
Author

Ajay-singh1 commented Mar 12, 2025

@craigbox The previous PR got cluttered up so I had to create a new one.Regarding the MD007 rule the ruby based linter doesnt strictly enforce the MD007 rule and allows mixed indentation while the node based linter enforces strict MD007 rule so a much better solution will be to disable the MD007 rule.Once the check passes here I will update the common files so that the change is applied to all istio repos.The linting is sucessful now:-

Screenshot from 2025-03-12 11-57-07

@dhawton
Copy link
Member

dhawton commented Mar 12, 2025

@craigbox The previous PR got cluttered up so I had to create a new one.Regarding the MD007 rule the ruby based linter doesnt strictly enforce the MD007 rule and allows mixed indentation while the node based linter enforces strict MD007 rule so a much better solution will be to disable the MD007 rule.Once the check passes here I will update the common files so that the change is applied to all istio repos.The linting is sucessful now:-

I actually strongly disagree with disabling MD007. This should be something we enforced, it's been something I have been fixing in PRs that I review, it'd be much better to enforce and resolve the issues.

@Ajay-singh1
Copy link
Author

@dhawton I need a couple of days to come up with a solution.Will be right back.

Copy link
Member

@Arhell Arhell left a comment

Choose a reason for hiding this comment

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

/ok-to-test

@istio-testing istio-testing added ok-to-test Set this label allow normal testing to take place for a PR not submitted by an Istio org member. and removed needs-ok-to-test labels Mar 13, 2025
@Ajay-singh1 Ajay-singh1 requested review from a team as code owners March 14, 2025 04:09
@istio-testing istio-testing added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 14, 2025
@Ajay-singh1
Copy link
Author

/retest

@Ajay-singh1
Copy link
Author

@dhawton markdownlint-cli2 is not being recognized.

@Ajay-singh1
Copy link
Author

/retest-required

@Ajay-singh1
Copy link
Author

Complex Issue unable to resolve closing this.

@Ajay-singh1 Ajay-singh1 deleted the pr-fix branch March 16, 2025 03:23
@Ajay-singh1 Ajay-singh1 restored the pr-fix branch March 19, 2025 17:09
@craigbox
Copy link
Contributor

@Ajay-singh1 are you intending to pick this up elsewhere? I think you have everything you need to proceed, and I can help steward it. Otherwise I can move it onto another maintainer.

@Ajay-singh1 Ajay-singh1 reopened this Mar 24, 2025
@Ajay-singh1
Copy link
Author

@craigbox Yes , this will work and will pass all the lint checks.I am waiting for the PR to be merged in istio/tools once it is approved the checks will pass here and we are good to go.

@craigbox
Copy link
Contributor

I don't see a PR. Can you link one if it exists? Else, do you want to add it to istio/tools#3161 ?
(I would also add both for the meantime)

@Ajay-singh1
Copy link
Author

Yes this specific PR I was talking about istio/tools#3161.It is in pending state once it merges the lint checks will pass here I am completely confident about it.

@Ajay-singh1
Copy link
Author

/retest

@craigbox
Copy link
Contributor

You may have to wait a day or so for the automator to build a new container and update this repo to use it.

@Ajay-singh1
Copy link
Author

👍

@Ajay-singh1
Copy link
Author

/retest

@craigbox
Copy link
Contributor

craigbox commented Mar 27, 2025 via email

@craigbox
Copy link
Contributor

Nice! The build is working.

The gencheck error is because you added a <!-- markdownlint-disable --> block to a code snippet, but not to the generated snips.sh file. However, I would suggest we shouldn't have an ignore block anywhere but the front matter of a document.

I would also like to standardize on using a markdownlint-disable: XXX block in the doc front matter. Can we see if this is natively supported or how we might need to introduce it?

@@ -81,6 +81,7 @@ spec:
mtls:
mode: STRICT
---
<!-- markdownlint-disable-file MD007 -->
Copy link
Contributor

Choose a reason for hiding this comment

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

This content is now in the YAML files that are used for testing, which will almost certainly cause all the tests to fail.

Copy link
Author

@Ajay-singh1 Ajay-singh1 Mar 27, 2025

Choose a reason for hiding this comment

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

I agree with you I will fix this within a day.After reviewing the documentation I found that it doesnt`t support rules in doc frontmatter disabling the rules through inline html comments is the optimal way.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, that's confirmed here: DavidAnson/markdownlint#1250

unfortunate.

@istio-testing
Copy link
Contributor

@Ajay-singh1: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
doc.test.profile-default_istio.io e3e7746 link true /test doc.test.profile-default
doc.test.profile-none_istio.io e3e7746 link true /test doc.test.profile-none

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

{
"default": false,
"MD002": { "level": 2 },
"MD007": { "indent": 4 },
Copy link
Contributor

Choose a reason for hiding this comment

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

This rule is disabled in many files. Can you look into why they are failing, and if we can just change the markup to pass correctly? Did we not have this rule in the old config?

Copy link
Author

Choose a reason for hiding this comment

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

We did have this rule in the old configuration.It is failing because markdownlint-cli2 enforces strict MD007 rules while the previous linter allows for mixed indentation for the MD007 rule.Changing markup will have several breaking changes.

@@ -0,0 +1,13 @@
{
"default": false,
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this turn off the default rules? Wouldn't we want them on?

Copy link
Author

Choose a reason for hiding this comment

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

Yes this will turn off the default rules , no turning them on is not a good way because all the rules will be applied by default which is used by the linter and we dont want them right?We will explicitly define the rules which we want to use to lint our markdown files.

@istio-testing istio-testing added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 29, 2025
@Ajay-singh1
Copy link
Author

I have addressed all the issues , We can cleanly merge this PR , If you have any further questions do let me know.Thanks @dhawton and @craigbox for your assistance.

@Ajay-singh1 Ajay-singh1 requested a review from craigbox March 30, 2025 14:43
@Ajay-singh1 Ajay-singh1 mentioned this pull request Apr 4, 2025
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ok-to-test Set this label allow normal testing to take place for a PR not submitted by an Istio org member. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Istio pages don't abide by the requirements of our linter
5 participants