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

Pack config: Process defaults and datastore values in patternProperties and additionalItems #5321

Merged
merged 22 commits into from
Jul 9, 2022

Conversation

cognifloyd
Copy link
Member

@cognifloyd cognifloyd commented Aug 6, 2021

Building on #5225 allowed pack config to get defaults populated and get datastore values injected when needed under additionalProperties

This PR does the same for patternProperties and additionalItems.

We're were not processing defaults or datastore values in pack config under:

This makes sure we are applying defaults on nested items in arrays as well.

TODO:

  • tests

@pull-request-size pull-request-size bot added the size/L PR that changes 100-499 lines. Requires some effort to review. label Aug 6, 2021
@cognifloyd cognifloyd self-assigned this Mar 31, 2022
@copart-jafloyd copart-jafloyd force-pushed the pack-config-more-jsonschema branch 2 times, most recently from 9475fe9 to 1b5d21a Compare March 31, 2022 19:12
It was trying to access lists as if they were dictionaries.
So, adjust to allow both dicts and lists.
@cognifloyd cognifloyd changed the title WIP Process defaults and datastore values in more pack config locations Pack config: Process defaults and datastore values in patternProperties and additionalItems Mar 31, 2022
@cognifloyd cognifloyd marked this pull request as ready for review March 31, 2022 19:32
@cognifloyd cognifloyd added this to the 3.7.0 milestone Mar 31, 2022
@cognifloyd cognifloyd requested a review from Kami March 31, 2022 19:33
Copy link
Contributor

@amanda11 amanda11 left a comment

Choose a reason for hiding this comment

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

This is great. I've not had chance to check the arrayItems changes, but went through patternProperties. Just raised a couple of minor comments on the patternProperties.

CHANGELOG.rst Outdated Show resolved Hide resolved
st2common/st2common/util/config_loader.py Outdated Show resolved Hide resolved
st2common/st2common/util/config_loader.py Outdated Show resolved Hide resolved
This documents the properties schema flattening algorithm with code comments.
That highlighted some vars and a method that could be renamed to clarify
the code even more.
Copy link
Contributor

@amanda11 amanda11 left a comment

Choose a reason for hiding this comment

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

Thanks for update, the additionalProperties handling is really clear now, and looks good.
I haven't looked at additionalItems, but will try and do that when you've added the extra tests.

Thanks for the refactor.

@pull-request-size pull-request-size bot added size/XL PR that changes 500-999 lines. Consider splitting work into several ones that easier to review. and removed size/L PR that changes 100-499 lines. Requires some effort to review. labels Jul 4, 2022
@cognifloyd
Copy link
Member Author

K. I added a test showing the order of precedence in of properties, patternProperties, and additionalProperties in pack config.

@cognifloyd
Copy link
Member Author

I'm reviewing the schema to make sure I implemented patternProperties correctly.

We use draft 4 (and draft 3's required):

"id": "http://json-schema.org/draft-04/schema#",

# https://github.com/json-schema/json-schema/blob/master/draft-04/schema
# The source material is licensed under the AFL or BSD license.
# Both draft 4 and custom schema has additionalProperties set to false by default.
# The custom schema differs from draft 4 with the extension of position, immutable,
# and draft 3 version of required.

In draft 4, patternProperties creates an all-of set of schemas for each property. Each property gets compared against all patterns. And it must match all of the schemas for the patterns it matches. So, we can't avoid compiling some of the patterns like I thought. 😥
https://datatracker.ietf.org/doc/html/draft-fge-json-schema-validation-00#section-8.3

So, I need to refactor a bit more to check all patterns against all keys not included in properties.

In draft 4, patternProperties creates an all-of set of schemas for each property. Each
property gets compared against all patterns. And it must match all of the schemas for
the patterns it matches. So, we cannot avoid compiling some of the patterns.
https://datatracker.ietf.org/doc/html/draft-fge-json-schema-validation-00#section-8.3
@cognifloyd cognifloyd force-pushed the pack-config-more-jsonschema branch from 95c6642 to 5931e6d Compare July 5, 2022 00:44
@cognifloyd
Copy link
Member Author

K. Refactored and ready for more reviews

@cognifloyd cognifloyd requested a review from amanda11 July 5, 2022 01:08
@cognifloyd cognifloyd force-pushed the pack-config-more-jsonschema branch 2 times, most recently from 66b2a98 to a6c9318 Compare July 5, 2022 02:00
@cognifloyd cognifloyd force-pushed the pack-config-more-jsonschema branch from a6c9318 to 1463f4d Compare July 5, 2022 02:23
Copy link
Contributor

@amanda11 amanda11 left a comment

Choose a reason for hiding this comment

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

this looks good to me.
The only thing I thought of was whether it was worth having additional additionalItems tests, e.g. are we covering the case where additionalItems is "true" and additionalIItems is "false".

I've approved though, if you don't think those are needed. As the logic looks good, and other than that tests look great.

Thanks for all the work.

@cognifloyd
Copy link
Member Author

With additionalItems: true, the schema for each of those additional items is effectively {}. So, there are:

  • no defaults to interpolate
  • no automatically decrypted secrets (because nothing has secret: true unda a {} schema.

But, I suppose there can still be jinja templates without auto-decryption. So, I just added a test for a template that uses |decrypt_kv

@cognifloyd cognifloyd force-pushed the pack-config-more-jsonschema branch from d3e517f to a5084e7 Compare July 5, 2022 17:21
@cognifloyd cognifloyd enabled auto-merge July 5, 2022 18:28
@cognifloyd cognifloyd requested a review from a team July 7, 2022 12:55
Copy link
Member

@rush-skills rush-skills left a comment

Choose a reason for hiding this comment

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

LGTM Code-wise, didn't get a chance to set up and test.

@cognifloyd cognifloyd merged commit f65487b into StackStorm:master Jul 9, 2022
@cognifloyd cognifloyd deleted the pack-config-more-jsonschema branch February 12, 2024 15:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug fix enhancement size/XL PR that changes 500-999 lines. Consider splitting work into several ones that easier to review.
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants