-
Notifications
You must be signed in to change notification settings - Fork 25
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
CHAINSAW-234 Add rhsm.environments to schema #145
CHAINSAW-234 Add rhsm.environments to schema #145
Conversation
Here are the System Profile validation results using Prod data.
Validating against this PR's spec:
|
Here are the System Profile validation results using Prod data.
Validating against this PR's spec:
|
Here are the System Profile validation results using Prod data.
Validating against this PR's spec:
|
fe76bb3
to
70c5363
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Tomboyo Looks good, but can we set a max length for the field? I was going to suggest 32 because of the examples, but one of your tests in valids
has a longer string
@Ceiu Right now the Insights schema treats environment (UU)IDs as opaque tokens without any specific format beyond being strings. Is there a maximum length we could enforce for the IDs, per Kruai's question? I think this would be used to reject malicious input, is that right @kruai ? In which case, this doesn't have to track closely with whatever the ID is implemented as, just be a tentative upper bound on how long it might be in the near future. |
@kruai I reached out to ceiu over slack and we landed on a maxLength of 256 to balance between hiding impl details from the schema and giving a reasonable upper-bound length. Let me know if that sounds good to you as well. Thanks. |
Here are the System Profile validation results using Prod data.
Validating against this PR's spec:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Tomboyo that sounds good to me. @thearifismail are you OK with me merging this?
Adds the rhsm.environments field to the schema. See associated change to insights-host-inventory, RedHatInsights/insights-host-inventory#2112, which reflects these changes there per the contrib section of the README.