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

Issue #530 - Use SafeLoader for YAML load calls #532

Merged
merged 1 commit into from
Aug 7, 2024
Merged

Conversation

nttoole
Copy link
Contributor

@nttoole nttoole commented Jul 17, 2024

The primary changes address using safer version of YAML loads:

  1. swapping yaml.load(source,Loader=yaml.Loader) to yaml.safe_load(source). (similar for load_all)
  2. swap Yaml object constructors from yaml.add_constructor() to yaml.SafeLoader.add_constructor()
  3. update pyyaml version to '6.0.1'

Then we hit a series of pre-commit errors due to newly-corrected configs which now run reorder-python-imports and black on the source code. So to satisfy those, more non-yaml updates were made. For example, AitConfigMissing was renamed AitConfigMissingError.
However, after a while, it became clear that black and flake8 have some contradictory rules ( e.g. 'binary logical operators across multiple lines' vs 'line-too-long' errors). In the end, we disabled checks for the binary-ops-spanning-multi-line checks in flake8, cuz that just seems a cleaner presentation (to me at least).
So there are a lot of line changes related to the above, had I known I would have just avoided satisfying any pre-commit issues and forced the Yaml-related changes only.

flake8 would still complain about source files which weren't even touched for the PR, so in the end the pre-commit was disabled so this PR could be made without touching nearly every source file (a separate PR for that busy-work).

Testing included automated tests, along with running the server with the AIT-GUI plugin and ait_example which successfully showed telemetry graphed in the browser. (Config file was reset to clear out plugins.)

Fixes #530

@nttoole nttoole requested review from a team as code owners July 17, 2024 16:57
@MJJoyce
Copy link
Member

MJJoyce commented Aug 7, 2024

Looks good to me. +1 to merge as is and resolve any linting / build issues separately.

@nttoole nttoole merged commit cc7e035 into master Aug 7, 2024
2 of 4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Multiple Local Code Executions (using YAML)
2 participants