Changing load/dump in source files #190641
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kc13greiner
added
Team:Security
|
/ci |
|
/ci |
|
/ci |
|
/ci |
|
/ci |
|
/ci |
1 similar comment
|
/ci |
|
/ci |
…row when calling safe functions.
|
/ci |
jeramysoucy
reviewed
Aug 20, 2024
| noRefs: true, | ||
| sortKeys: sortYamlKeys, | ||
| skipInvalid: true, // Skip invalid types like `undefined` |
There was a problem hiding this comment.
Adding skipInvalid: true allows the files to be written even if they contain invalid types (e.g. undefined). The result is that the invalid types will not be written to the file rather than the default behavior of throwing an exception.
Previous use of the dump function would write files containing invalid types. It looks like we relied on parsing the file contents to handling removing the invalid values, which may no longer be necessary.
jeramysoucy
reviewed
Aug 20, 2024
Comment on lines
111
to
112
| // As we have switched to using the js-yaml safeDump function, we are stripping | ||
| // out invalid types like 'undefined' when the spec is written to file. |
There was a problem hiding this comment.
To reviewers: does it make sense to refactor this test, or rather to implement unit tests for the write_yaml_document.ts utility? The end result is the same, hence this test passes, but the behavior at the utility level is different.
There was a problem hiding this comment.
In fact write_yaml_document.ts contained a hack to remove undefined via JSON.parse(JSON.stringinfy()). I removed this hack so the result behavior stays the same.
The output of this utility is committed to the repo so we can immediately spot any inconsistencies. It also means that having a lot of unit tests to verify all aspects isn't strictly necessary.
|
/ci |
shahzad31
approved these changes
Aug 20, 2024
seanrathier
approved these changes
Aug 20, 2024
tomsonpl
approved these changes
Aug 20, 2024
thomheymann
approved these changes
Aug 20, 2024
|
/ci |
smith
approved these changes
Aug 20, 2024
maximpn
approved these changes
Aug 21, 2024
There was a problem hiding this comment.
@kc13greiner @jeramysoucy thank you for your effort in preparation for upgrading js-yaml to a newer version 🙏
I pushed changes made in kbn-openapi-bunder to simplify the implementation and make sure it conforms expected behavior. Since the output is committed to the repo it should be instantly visible if some inconsistencies are introduced (I haven't spot any). safeLoad shouldn't break anything too.
I considered upgrading js-yaml to be consumed in kbn-openapi-bunder but it required additional effort I didn't have capacity for.
Comment on lines
111
to
112
| // As we have switched to using the js-yaml safeDump function, we are stripping | ||
| // out invalid types like 'undefined' when the spec is written to file. |
There was a problem hiding this comment.
In fact write_yaml_document.ts contained a hack to remove undefined via JSON.parse(JSON.stringinfy()). I removed this hack so the result behavior stays the same.
The output of this utility is committed to the repo so we can immediately spot any inconsistencies. It also means that having a lot of unit tests to verify all aspects isn't strictly necessary.
💛 Build succeeded, but was flaky
Failed CI StepsMetrics [docs]Async chunks
History
To update your PR or re-run it, just comment with: cc @jeramysoucy |
💔 All backports failed
Manual backportTo create the backport manually run: Questions ?Please refer to the Backport tool documentation |
kc13greiner
added a commit
to kc13greiner/kibana
that referenced
this pull request
Aug 21, 2024
## Summary Updates usage of `js-yaml` `load` and `dump` to `safeLoad` and `safeDump`, in preparation for a major version update of dependency, where the default behavior will be that of the safe function variants. ## Note to reviewers `safeDump` will throw if it encounters invalid types (e.g. `undefined`), whereas the `dump` function will still write the file including the invalid types. This may have an affect within your use cases - if throwing is not acceptable or is unhandled. To avoid this the `skipInvalid` option can be used (see https://github.com/nodeca/js-yaml#dump-object---options-) - this will write the file, stripping out any invalid types from the input. Please consider this when reviewing the changes to your code. If the `skipInvalid` option is needed, please add it, or let us know to make the change. --------- Co-authored-by: Sid <[email protected]> Co-authored-by: “jeramysoucy” <[email protected]> Co-authored-by: Elena Shostak <[email protected]> Co-authored-by: kibanamachine <[email protected]> Co-authored-by: Maxim Palenov <[email protected]> (cherry picked from commit bcc46b6) # Conflicts: # .buildkite/pipeline-resource-definitions/scripts/fix-location-collection.ts # scripts/enabled_ftr_configs.js
kc13greiner
added a commit
to kc13greiner/kibana
that referenced
this pull request
Aug 21, 2024
## Summary Updates usage of `js-yaml` `load` and `dump` to `safeLoad` and `safeDump`, in preparation for a major version update of dependency, where the default behavior will be that of the safe function variants. ## Note to reviewers `safeDump` will throw if it encounters invalid types (e.g. `undefined`), whereas the `dump` function will still write the file including the invalid types. This may have an affect within your use cases - if throwing is not acceptable or is unhandled. To avoid this the `skipInvalid` option can be used (see https://github.com/nodeca/js-yaml#dump-object---options-) - this will write the file, stripping out any invalid types from the input. Please consider this when reviewing the changes to your code. If the `skipInvalid` option is needed, please add it, or let us know to make the change. --------- Co-authored-by: Sid <[email protected]> Co-authored-by: “jeramysoucy” <[email protected]> Co-authored-by: Elena Shostak <[email protected]> Co-authored-by: kibanamachine <[email protected]> Co-authored-by: Maxim Palenov <[email protected]> (cherry picked from commit bcc46b6) # Conflicts: # .buildkite/pipeline-resource-definitions/scripts/fix-location-collection.ts # .buildkite/pipeline-utils/agent_images.ts # .buildkite/pipeline-utils/buildkite/client.ts # .buildkite/pipeline-utils/ci-stats/pick_test_group_run_order.ts # packages/kbn-es/src/utils/read_roles_from_resource.ts # packages/kbn-eslint-config/.eslintrc.js # packages/kbn-eslint-plugin-eslint/index.js # packages/kbn-openapi-bundler/src/utils/extract_by_json_pointer.ts # packages/kbn-openapi-bundler/src/utils/read_document.ts # packages/kbn-openapi-bundler/src/utils/write_yaml_document.ts # packages/kbn-openapi-bundler/tests/bundler/bundle_specs.ts # packages/kbn-openapi-bundler/tests/bundler/bundle_specs_with_multiple_modifications.test.ts # packages/kbn-openapi-bundler/tests/bundler/circular.test.ts # packages/kbn-openapi-bundler/tests/merger/merge_specs.ts # scripts/enabled_ftr_configs.js # src/dev/build/tasks/os_packages/create_os_package_kibana_yml.ts # x-pack/packages/kbn-data-forge/src/lib/create_config.ts # x-pack/plugins/cloud_defend/common/utils/helpers.ts # x-pack/plugins/cloud_defend/public/components/control_general_view/index.test.tsx # x-pack/plugins/observability_solution/apm/public/components/fleet_integration/apm_agents/runtime_attachment/supported_agents/java_runtime_attachment.tsx # x-pack/plugins/observability_solution/apm/server/routes/fleet/get_apm_package_policy_definition.ts # x-pack/plugins/observability_solution/observability_onboarding/common/elastic_agent_logs/custom_logs/generate_custom_logs_yml.test.ts # x-pack/plugins/observability_solution/observability_onboarding/common/elastic_agent_logs/custom_logs/generate_custom_logs_yml.ts # x-pack/plugins/observability_solution/observability_onboarding/common/elastic_agent_logs/system_logs/generate_system_logs_yml.ts # x-pack/plugins/observability_solution/observability_onboarding/server/routes/flow/route.ts # x-pack/plugins/security_solution/scripts/endpoint/common/elastic_agent_service.ts # x-pack/test/observability_onboarding_api_integration/tests/elastic_agent/config.spec.ts
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation |
jeramysoucy
pushed a commit
that referenced
this pull request
Aug 22, 2024
# Backport This will backport the following commits from `main` to `8.15`: - [Changing load/dump in source files (#190641)](#190641) <!--- Backport version: 8.9.8 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) <!--BACKPORT [{"author":{"name":"Kurt","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-08-21T11:29:36Z","message":"Changing load/dump in source files (#190641)\n\n## Summary\r\n\r\nUpdates usage of `js-yaml` `load` and `dump` to `safeLoad` and\r\n`safeDump`, in preparation for a major version update of dependency,\r\nwhere the default behavior will be that of the safe function variants.\r\n\r\n\r\n## Note to reviewers\r\n`safeDump` will throw if it encounters invalid types (e.g. `undefined`),\r\nwhereas the `dump` function will still write the file including the\r\ninvalid types. This may have an affect within your use cases - if\r\nthrowing is not acceptable or is unhandled. To avoid this the\r\n`skipInvalid` option can be used (see\r\nhttps://github.com/nodeca/js-yaml#dump-object---options-) - this will\r\nwrite the file, stripping out any invalid types from the input.\r\n\r\nPlease consider this when reviewing the changes to your code. If the\r\n`skipInvalid` option is needed, please add it, or let us know to make\r\nthe change.\r\n\r\n---------\r\n\r\nCo-authored-by: Sid <[email protected]>\r\nCo-authored-by: “jeramysoucy” <[email protected]>\r\nCo-authored-by: Elena Shostak <[email protected]>\r\nCo-authored-by: kibanamachine <[email protected]>\r\nCo-authored-by: Maxim Palenov <[email protected]>","sha":"bcc46b60e99ddb3c86f64f794296405064334335","branchLabelMapping":{"^v8.16.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["Team:Security","release_note:skip","backport:all-open","ci:project-deploy-observability","Team:obs-ux-infra_services","risk","v8.16.0","v8.15.1"],"number":190641,"url":"https://github.com/elastic/kibana/pull/190641","mergeCommit":{"message":"Changing load/dump in source files (#190641)\n\n## Summary\r\n\r\nUpdates usage of `js-yaml` `load` and `dump` to `safeLoad` and\r\n`safeDump`, in preparation for a major version update of dependency,\r\nwhere the default behavior will be that of the safe function variants.\r\n\r\n\r\n## Note to reviewers\r\n`safeDump` will throw if it encounters invalid types (e.g. `undefined`),\r\nwhereas the `dump` function will still write the file including the\r\ninvalid types. This may have an affect within your use cases - if\r\nthrowing is not acceptable or is unhandled. To avoid this the\r\n`skipInvalid` option can be used (see\r\nhttps://github.com/nodeca/js-yaml#dump-object---options-) - this will\r\nwrite the file, stripping out any invalid types from the input.\r\n\r\nPlease consider this when reviewing the changes to your code. If the\r\n`skipInvalid` option is needed, please add it, or let us know to make\r\nthe change.\r\n\r\n---------\r\n\r\nCo-authored-by: Sid <[email protected]>\r\nCo-authored-by: “jeramysoucy” <[email protected]>\r\nCo-authored-by: Elena Shostak <[email protected]>\r\nCo-authored-by: kibanamachine <[email protected]>\r\nCo-authored-by: Maxim Palenov <[email protected]>","sha":"bcc46b60e99ddb3c86f64f794296405064334335"}},"sourceBranch":"main","suggestedTargetBranches":["8.15"],"targetPullRequestStates":[{"branch":"main","label":"v8.16.0","labelRegex":"^v8.16.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/190641","number":190641,"mergeCommit":{"message":"Changing load/dump in source files (#190641)\n\n## Summary\r\n\r\nUpdates usage of `js-yaml` `load` and `dump` to `safeLoad` and\r\n`safeDump`, in preparation for a major version update of dependency,\r\nwhere the default behavior will be that of the safe function variants.\r\n\r\n\r\n## Note to reviewers\r\n`safeDump` will throw if it encounters invalid types (e.g. `undefined`),\r\nwhereas the `dump` function will still write the file including the\r\ninvalid types. This may have an affect within your use cases - if\r\nthrowing is not acceptable or is unhandled. To avoid this the\r\n`skipInvalid` option can be used (see\r\nhttps://github.com/nodeca/js-yaml#dump-object---options-) - this will\r\nwrite the file, stripping out any invalid types from the input.\r\n\r\nPlease consider this when reviewing the changes to your code. If the\r\n`skipInvalid` option is needed, please add it, or let us know to make\r\nthe change.\r\n\r\n---------\r\n\r\nCo-authored-by: Sid <[email protected]>\r\nCo-authored-by: “jeramysoucy” <[email protected]>\r\nCo-authored-by: Elena Shostak <[email protected]>\r\nCo-authored-by: kibanamachine <[email protected]>\r\nCo-authored-by: Maxim Palenov <[email protected]>","sha":"bcc46b60e99ddb3c86f64f794296405064334335"}},{"branch":"8.15","label":"v8.15.1","labelRegex":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"}]}] BACKPORT--> --------- Co-authored-by: kibanamachine <[email protected]>
jeramysoucy
pushed a commit
that referenced
this pull request
Aug 22, 2024
# Backport This will backport the following commits from `main` to `7.17`: - [Changing load/dump in source files (#190641)](#190641) <!--- Backport version: 8.9.8 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) <!--BACKPORT [{"author":{"name":"Kurt","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-08-21T11:29:36Z","message":"Changing load/dump in source files (#190641)\n\n## Summary\r\n\r\nUpdates usage of `js-yaml` `load` and `dump` to `safeLoad` and\r\n`safeDump`, in preparation for a major version update of dependency,\r\nwhere the default behavior will be that of the safe function variants.\r\n\r\n\r\n## Note to reviewers\r\n`safeDump` will throw if it encounters invalid types (e.g. `undefined`),\r\nwhereas the `dump` function will still write the file including the\r\ninvalid types. This may have an affect within your use cases - if\r\nthrowing is not acceptable or is unhandled. To avoid this the\r\n`skipInvalid` option can be used (see\r\nhttps://github.com/nodeca/js-yaml#dump-object---options-) - this will\r\nwrite the file, stripping out any invalid types from the input.\r\n\r\nPlease consider this when reviewing the changes to your code. If the\r\n`skipInvalid` option is needed, please add it, or let us know to make\r\nthe change.\r\n\r\n---------\r\n\r\nCo-authored-by: Sid <[email protected]>\r\nCo-authored-by: “jeramysoucy” <[email protected]>\r\nCo-authored-by: Elena Shostak <[email protected]>\r\nCo-authored-by: kibanamachine <[email protected]>\r\nCo-authored-by: Maxim Palenov <[email protected]>","sha":"bcc46b60e99ddb3c86f64f794296405064334335","branchLabelMapping":{"^v8.16.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["Team:Security","release_note:skip","backport:all-open","ci:project-deploy-observability","Team:obs-ux-infra_services","risk","v8.16.0","v8.15.1"],"number":190641,"url":"https://github.com/elastic/kibana/pull/190641","mergeCommit":{"message":"Changing load/dump in source files (#190641)\n\n## Summary\r\n\r\nUpdates usage of `js-yaml` `load` and `dump` to `safeLoad` and\r\n`safeDump`, in preparation for a major version update of dependency,\r\nwhere the default behavior will be that of the safe function variants.\r\n\r\n\r\n## Note to reviewers\r\n`safeDump` will throw if it encounters invalid types (e.g. `undefined`),\r\nwhereas the `dump` function will still write the file including the\r\ninvalid types. This may have an affect within your use cases - if\r\nthrowing is not acceptable or is unhandled. To avoid this the\r\n`skipInvalid` option can be used (see\r\nhttps://github.com/nodeca/js-yaml#dump-object---options-) - this will\r\nwrite the file, stripping out any invalid types from the input.\r\n\r\nPlease consider this when reviewing the changes to your code. If the\r\n`skipInvalid` option is needed, please add it, or let us know to make\r\nthe change.\r\n\r\n---------\r\n\r\nCo-authored-by: Sid <[email protected]>\r\nCo-authored-by: “jeramysoucy” <[email protected]>\r\nCo-authored-by: Elena Shostak <[email protected]>\r\nCo-authored-by: kibanamachine <[email protected]>\r\nCo-authored-by: Maxim Palenov <[email protected]>","sha":"bcc46b60e99ddb3c86f64f794296405064334335"}},"sourceBranch":"main","suggestedTargetBranches":["8.15"],"targetPullRequestStates":[{"branch":"main","label":"v8.16.0","labelRegex":"^v8.16.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/190641","number":190641,"mergeCommit":{"message":"Changing load/dump in source files (#190641)\n\n## Summary\r\n\r\nUpdates usage of `js-yaml` `load` and `dump` to `safeLoad` and\r\n`safeDump`, in preparation for a major version update of dependency,\r\nwhere the default behavior will be that of the safe function variants.\r\n\r\n\r\n## Note to reviewers\r\n`safeDump` will throw if it encounters invalid types (e.g. `undefined`),\r\nwhereas the `dump` function will still write the file including the\r\ninvalid types. This may have an affect within your use cases - if\r\nthrowing is not acceptable or is unhandled. To avoid this the\r\n`skipInvalid` option can be used (see\r\nhttps://github.com/nodeca/js-yaml#dump-object---options-) - this will\r\nwrite the file, stripping out any invalid types from the input.\r\n\r\nPlease consider this when reviewing the changes to your code. If the\r\n`skipInvalid` option is needed, please add it, or let us know to make\r\nthe change.\r\n\r\n---------\r\n\r\nCo-authored-by: Sid <[email protected]>\r\nCo-authored-by: “jeramysoucy” <[email protected]>\r\nCo-authored-by: Elena Shostak <[email protected]>\r\nCo-authored-by: kibanamachine <[email protected]>\r\nCo-authored-by: Maxim Palenov <[email protected]>","sha":"bcc46b60e99ddb3c86f64f794296405064334335"}},{"branch":"8.15","label":"v8.15.1","labelRegex":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"}]}] BACKPORT-->
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Updates usage of
js-yamlloadanddumptosafeLoadandsafeDump, in preparation for a major version update of dependency, where the default behavior will be that of the safe function variants.Closes #190624
Note to reviewers
safeDumpwill throw if it encounters invalid types (e.g.undefined), whereas thedumpfunction will still write the file including the invalid types. This may have an affect within your use cases - if throwing is not acceptable or is unhandled. To avoid this theskipInvalidoption can be used (see https://github.com/nodeca/js-yaml#dump-object---options-) - this will write the file, stripping out any invalid types from the input.Please consider this when reviewing the changes to your code. If the
skipInvalidoption is needed, please add it, or let us know to make the change.