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

Linting controls #1063

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Linting controls #1063

wants to merge 5 commits into from

Conversation

dkolbly
Copy link
Contributor

@dkolbly dkolbly commented Apr 24, 2024

Related Issue:

#1061 Support linting of enum and sibling conventions

Description of changes:

  • Adds a suppress_checks option to the metaschema to configure turning off certain linting rules
  • Turns off those linting checks for places where we have violated the conventions (there are about 3)
  • Fixes data_lifecycle_state_id to follow the enum convention by adding a 99 (Other) enumerand and articulating that it should be used for "other"

@dkolbly dkolbly added documentation Improvements or additions to documentation framework Structures, conventions, requirements, data types, etc. labels Apr 24, 2024
@dkolbly dkolbly self-assigned this Apr 24, 2024
This commit introduces a notion of "lint" setting, which is to be used
to configure linting behavior for tools that can automatically
validate that conventions are being followed.

Also marks several historical attributes as behaving unconventionally,
specifically:

   - activity_id's sibling is called activity_name instead of activity
   - the enums for opcode_id and disk_level_id enum are based on
     external standards and hence do not follow the 0/99 convention

Signed-off-by: Donovan Kolbly <donovan@rscheme.org>
Signed-off-by: Donovan Kolbly <donovan@rscheme.org>
@dkolbly
Copy link
Contributor Author

dkolbly commented Apr 24, 2024

I will note that the validator in ocsf-validator does not yet understand lint and hence is failing against this PR. If discussion proceeds, I can probably address that issue as well at least at a surface level.

@rmouritzen-splunk
Copy link
Contributor

The ocsf/ocsf-validator needs a change to support the new lint attribute. I believe the change will be in ocsf_validator/types.py, adding a lint key to OcsfAttr:

OcsfAttr = TypedDict(
    "OcsfAttr",
    {
        "$include": NotRequired[str],
        # "caption": NotRequired[str],
        "caption": str,
        "default": NotRequired[Any],
        "description": NotRequired[str],
        "enum": NotRequired[Dict[str, OcsfEnumMember]],
        "group": NotRequired[str],
        "is_array": NotRequired[bool],
        "lint": NotRequired[Sequence[str]],
        "max_len": NotRequired[int],
        "name": NotRequired[str],
        "notes": NotRequired[str],
        "observable": NotRequired[int],
        "range": NotRequired[Sequence[int]],
        "regex": NotRequired[str],
        "requirement": NotRequired[str],
        "sibling": NotRequired[str],
        "type": NotRequired[str],
        "type_name": NotRequired[str],
        "profile": NotRequired[Optional[Sequence[str]]],
        "values": NotRequired[Sequence[Any]],
        "@deprecated": NotRequired[OcsfDeprecationInfo],
    },
)

(I say "I believe..." because I haven't tried this change against the modified schema myself.)

@dkolbly
Copy link
Contributor Author

dkolbly commented May 2, 2024

I will try it out!

@alanisaac
Copy link
Contributor

Do we have rules for naming conventions for fields like this? Thinking about:

  • We use a $ prefix for references to other fields (which is similar to JSON schema's conventions, as well as comments)
  • We also have a @ prefix in @deprecated as an annotation for the schema. I don't know the rationale behind that prefix, but it reminds me of a Python decorator (such as @deprecated here).

Should lint follow one of these conventions (@lint or $lint) since it's more of a validation directive or annotation than something that influences the final schemas themselves?

@dkolbly
Copy link
Contributor Author

dkolbly commented May 7, 2024

@alanisaac great question... I don't have an opinion on that, really. There are lots of other properties that directed at the compiler (i.e., that do not appear in the produced concrete schema, e.g., JSONSchema) like extends and the top level name property in files, so I didn't think anything of using an undecorated property name for lint. The notion of linting the schema itself is new, so I'm not sure where else to look for a comparative example...

@rmouritzen-splunk
Copy link
Contributor

rmouritzen-splunk commented Jun 4, 2024

I see a couple things:

  1. The suppressions use skewer-case instead of snake_case. Perhaps they should be snake_case for consistency.
  2. It's not clear to me what check each is actually suppressing. Can these be described somewhere?

(I'm quite interested because I'm adding a new event validation API, and I'll want to add support for these suppressions to that API.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation framework Structures, conventions, requirements, data types, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants