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

ValueError exception using --ref #892

Closed
ebeahan opened this issue Jul 22, 2020 · 2 comments · Fixed by #937
Closed

ValueError exception using --ref #892

ebeahan opened this issue Jul 22, 2020 · 2 comments · Fixed by #937
Assignees
Labels
1.7.0 bug Something isn't working ready Issues we'd like to address in the future.

Comments

@ebeahan
Copy link
Member

ebeahan commented Jul 22, 2020

Description of the problem including expected versus actual behavior:

Running the generator.py with the --ref option with a tag or commit prior to d8a5e2a will fail with a ValueError exception. Using git checkout <tag> continues to work.

Steps to reproduce:

$ python scripts/generator.py --ref v1.5.0

Provide logs (if relevant):

Working:

$ python scripts/generator.py --ref d8a5e2a
Loading schemas from git ref d8a5e2a
Running generator. ECS version 1.6.0-dev

Failing:

git tag:

$ python scripts/generator.py --ref v1.5.0
Loading schemas from git ref v1.5.0
Running generator. ECS version 1.5.0
Traceback (most recent call last):
  File "scripts/generator.py", line 94, in <module>
    main()
  File "scripts/generator.py", line 44, in main
    cleaner.clean(fields)
  File "<redacted>/ecs/scripts/schema/cleaner.py", line 23, in clean
    visitor.visit_fields(fields, fieldset_func=schema_cleanup, field_func=field_cleanup)
  File "<redacted>/ecs/scripts/schema/visitor.py", line 21, in visit_fields
    visit_fields(details['fields'],
  File "<redacted>/ecs/scripts/schema/visitor.py", line 19, in visit_fields
    field_func(details)
  File "<redacted>/ecs/scripts/schema/cleaner.py", line 125, in field_cleanup
    field_assertions_and_warnings(field)
  File "<redacted>/ecs/scripts/schema/cleaner.py", line 168, in field_assertions_and_warnings
    single_line_short_description(field)
  File "<redacted>/ecs/scripts/schema/cleaner.py", line 189, in single_line_short_description
    raise ValueError(msg)
ValueError: Short descriptions must be single line, and under 120 characters (current length: 134).
Offending field or field set: number
Short description:
  Unique number allocated to the autonomous system. The autonomous system number (ASN) uniquely identifies each network on the Internet.

git commit sha:

$ python scripts/generator.py --ref 75da22b
Loading schemas from git ref 75da22b
Running generator. ECS version 1.6.0-dev
Traceback (most recent call last):
  File "scripts/generator.py", line 94, in <module>
    main()
  File "scripts/generator.py", line 44, in main
    cleaner.clean(fields)
  File "<redacted>/ecs/scripts/schema/cleaner.py", line 23, in clean
    visitor.visit_fields(fields, fieldset_func=schema_cleanup, field_func=field_cleanup)
  File "<redacted>/ecs/scripts/schema/visitor.py", line 21, in visit_fields
    visit_fields(details['fields'],
  File "<redacted>/ecs/scripts/schema/visitor.py", line 19, in visit_fields
    field_func(details)
  File "<redacted>/ecs/scripts/schema/cleaner.py", line 125, in field_cleanup
    field_assertions_and_warnings(field)
  File "<redacted>/ecs/scripts/schema/cleaner.py", line 168, in field_assertions_and_warnings
    single_line_short_description(field)
  File "<redacted>/ecs/scripts/schema/cleaner.py", line 189, in single_line_short_description
    raise ValueError(msg)
ValueError: Short descriptions must be single line, and under 120 characters (current length: 134).
Offending field or field set: number
Short description:
  Unique number allocated to the autonomous system. The autonomous system number (ASN) uniquely identifies each network on the Internet.

Any additional context:

The short descriptions were fixed in a91aa11 to align with the new length checking of short, but previously fields missing short fall back to the description field which is failing the new short character length check. Need to investigate how to improve --ref to handle this change in the generator.

@ebeahan ebeahan added the bug Something isn't working label Jul 22, 2020
@ebeahan ebeahan self-assigned this Jul 22, 2020
@webmat
Copy link
Contributor

webmat commented Jul 22, 2020

Oh jeez we're getting into meta-land 😂

As you point out, since #871 ECS tools are now stricter in validating short descriptions. That commit id is the merge commit for that PR. This became necessary after a subtle issue in #762 that had the short description as a full paragraph. Although when working on #871 I did adjust a lot of descriptions, so --ref probably doesn't work at all on anything prior to #871.

I'm not 100% sure what the right answer is, here.

I do want the main build to fail if short descriptions are not actually short. But anyone building custom artifacts based on --ref, --include and --subset should be able to get their work done as well...

Here's a few thoughts:

  • Approach this simply and disable this specific error when someone is running the tools with --ref.
  • Add a flag to ignore all validation errors. This sounds a bit blunt; on the other hand, this transition point where we added this validation will fade into the past, as we keep releasing ECS versions.
  • Add a flag that turns all validation errors into warnings instead.
  • Add a --strict flag and use it for the main build. Some of these validation errors could be run only when in strict mode.

@webmat
Copy link
Contributor

webmat commented Jul 22, 2020

And this reminds me that we would benefit from writing a few tests that run the CLI tool, to catch things like that. The idea was floated here a while back #822 (comment)

@webmat webmat added ready Issues we'd like to address in the future. 1.6.0 labels Aug 11, 2020
@ebeahan ebeahan added 1.7.0 and removed 1.6.0 labels Aug 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.7.0 bug Something isn't working ready Issues we'd like to address in the future.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants