Skip to content

Conversation

OVI3D0
Copy link
Member

@OVI3D0 OVI3D0 commented Aug 12, 2025

Description

Adds a lint checker for both 1.x and 2.x terms. If the check-1.x-terms flag is passed then the lint checker will check for any 1.x terms, which include:

TERMS_1X = [
    "provision-configs",
    "provision-config-instances",
    "results-publishing",
    "results-publisher",
    "load-worker-coordinator-hosts",
    "node-ip",
    "coordinator-ip",
    "execute-test",
]

And if the check-2.x-terms flag is passed, it'll check for:

TERMS_2X = [
    "cluster-configs",
    "reporting",
    "worker-hosts",
    "run-test",
    "test-run",
]

Issues Resolved

[List any issues this PR will resolve]

Testing

  • New functionality includes testing

[Describe how this change was tested]


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@OVI3D0 OVI3D0 added the check-1.x-terms Label that runs a lint checker to check for 1.x terms. Fails if 1.x terms are found in the codebase. label Aug 12, 2025

VARIANT_PATTERNS = []
SKIP_DIRS = {".git", "venv", "__pycache__", ".pytest_cache"}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's skip ./ci directories too.

[Forbidden Term] ./.ci/scripts/check_deprecated_terms.py:9: "provision-configs",
[Forbidden Term] ./.ci/scripts/check_deprecated_terms.py:10: "provision-config-instances",
[Forbidden Term] ./.ci/scripts/check_deprecated_terms.py:11: "results-publishing",
[Forbidden Term] ./.ci/scripts/check_deprecated_terms.py:12: "results-publisher",
[Forbidden Term] ./.ci/scripts/check_deprecated_terms.py:13: "load-worker-coordinator-hosts",
[Forbidden Term] ./.ci/scripts/check_deprecated_terms.py:14: "execute-test",

Any ideas on how we can skip some of these too?

[Forbidden Term] ./tests/worker_coordinator/worker_coordinator_test.py:2052: async def test_execute_request_success(self):
[Forbidden Term] ./tests/worker_coordinator/worker_coordinator_test.py:2084: async def test_execute_request_with_throttling(self):
[Forbidden Term] ./osbenchmark/benchmark.py:1234: DEPRECATED_SUBCOMMANDS = ["execute-test", "execute"]

Copy link
Member Author

@OVI3D0 OVI3D0 Aug 13, 2025

Choose a reason for hiding this comment

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

hmm would you want to skip unit tests dir as well @IanHoang ?

Copy link
Collaborator

@IanHoang IanHoang left a comment

Choose a reason for hiding this comment

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

Left one comment

@IanHoang
Copy link
Collaborator

Update: Had offline chat with Michael.

We should add ways to bypass lines with certain comments, similar to PyPi. If users present check-deprecated-terms-disable-1x or check-deprecated-terms-2x above or next to the offending line, the script will not register this line as an offending line.

    # check-deprecated-terms-disable-1x
    DEPRECATED_SUBCOMMANDS = ["execute-test", "execute"]

To do this, just add this to script:

# Add this dictionary at the top
SUPRESS_MARKERS = {
    "block-1x": "check-deprecated-terms-disable-1x",
    "block-2x": "check-deprecated-terms-disable-2x"
}

# Update walk_and_check to take in a mode and add if statement that tracks comment suppression markers
def walk_and_check(patterns: list[re.Pattern], mode: str) -> int:
    error_found = 0
    suppress_marker = SUPRESS_MARKERS.get(mode)
    for root, _, files in os.walk("."):
        if any(skip in root.split(os.sep) for skip in SKIP_DIRS):
            continue
        for f in files:
            full_path = os.path.join(root, f)
            if not should_check_file(full_path):
                continue
            try:
                with open(full_path, "r", encoding="utf-8") as fh:
                    previous_line = ""
                    for i, line in enumerate(fh, 1):
                        if suppress_marker in previous_line or suppress_marker in line:
                            previous_line = line
                            continue
                        for patt in patterns:
                            if patt.search(line):
                                print(f"[Forbidden Term] {full_path}:{i}: {line.strip()}")
                                error_found = 1
                                break
            except Exception as e:
                print(f"[Warning] Skipped file {full_path}: {e}")
    return error_found

# Update
    failed = walk_and_check(patterns, mode)

Ran it locally with these changes and got no more errors.

"""
CmdLineProgressResultsPublisher supports displaying an updating progress indication together with an information message.
CmdLineProgressResultsPublisher supports displaying an updating progress indication together with an information message.
Copy link
Collaborator

Choose a reason for hiding this comment

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

@OVI3D0 Can you remove this trailing white space to unblock the lint checker?

Copy link
Member Author

Choose a reason for hiding this comment

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

Should be fixed now 👍

class CmdLineProgressResultsPublisher:
# check-deprecated-terms-disable-1x
"""
CmdLineProgressResultsPublisher supports displaying an updating progress indication together with an information message.
Copy link
Collaborator

Choose a reason for hiding this comment

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

@OVI3D0 Can you remove comment on line 175 and just replace CmdLineProgressResultsPublisher on line 177 with "This class supports..."

Copy link
Collaborator

@IanHoang IanHoang left a comment

Choose a reason for hiding this comment

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

LGTM

@IanHoang IanHoang merged commit e9d04ad into opensearch-project:2.0-beta Aug 18, 2025
11 of 14 checks passed
gkamat pushed a commit to gkamat/opensearch-benchmark that referenced this pull request Aug 18, 2025
gkamat pushed a commit that referenced this pull request Aug 19, 2025
IanHoang pushed a commit that referenced this pull request Aug 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
check-1.x-terms Label that runs a lint checker to check for 1.x terms. Fails if 1.x terms are found in the codebase.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants