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

CHAINSAW-234 Add environments to schema #2112

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Tomboyo
Copy link

@Tomboyo Tomboyo commented Dec 4, 2024

Overview

Adds the rhsm.environments field to the system profile schema, per CONTRIBUTING guidelines of inventory-schemas. See RedHatInsights/inventory-schemas#145. Let me know if I'm issing anything!

PR Checklist

  • Keep PR title short, ideally under 72 characters
  • Descriptive comments provided in complex code blocks
  • Include raw query examples in the PR description, if adding/modifying SQL query
  • Tests: validate optimal/expected output
  • Tests: validate exceptions and failure scenarios
  • Tests: edge cases
  • Recovers or fails gracefully during potential resource outages (e.g. DB, Kafka)
  • Uses type hinting, if convenient
  • Documentation, if this PR changes the way other services interact with host inventory
  • Links to related PRs

Secure Coding Practices Documentation Reference

You can find documentation on this checklist here.

Secure Coding Checklist

  • Input Validation
  • Output Encoding
  • Authentication and Password Management
  • Session Management
  • Access Control
  • Cryptographic Practices
  • Error Handling and Logging
  • Data Protection
  • Communication Security
  • System Configuration
  • Database Security
  • File Management
  • Memory Management
  • General Coding Practices

@@ -592,6 +592,15 @@ $defs:
example: "8.1, 7.5, 9.9"
type: string
maxLength: 255
environments:
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this attribute be renamed to indicate its purpose? "environment" appears to generic and can raise questions in future, if not now?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Just to clarify, this field is under rhsm, so it's rhsm.environments. I'm not against a more specific field name, but it at least won't conflict with fields from other workloads

Copy link
Author

@Tomboyo Tomboyo Dec 5, 2024

Choose a reason for hiding this comment

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

The other option for a name would be rhsm.content_templates, which aligns with what Insights refers to them as (as I understand it). Hosted Candlepin (the source of truth for this data) refers to them as environments, though. Is either of those clearer than the others?

Copy link
Author

Choose a reason for hiding this comment

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

I also see other schema entries tend to suffix _uuid or _id, but not all of them. Do we have a preference on way or the other?

type: array
items:
description: An environment UUID.
type: string
Copy link
Contributor

Choose a reason for hiding this comment

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

Try setting type to UUID as it is a type of fields supported mashmallow but not sure if openapi specs allow it. If not then use:

type: string
format: uuid
required: true | false

Copy link
Collaborator

Choose a reason for hiding this comment

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

UUID is a valid value for format, but not for type (source). However, I think that format requires hyphenated UUIDs.

Copy link
Author

Choose a reason for hiding this comment

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

To the best of my understanding, UUID isn't a formally defined OpenAPI type. Other uuid-type fields in the spec file seemed to use no-format string types and a pattern regex.

Copy link
Contributor

Choose a reason for hiding this comment

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

As @kruai said, UUID is NOT a type but format: uuid is. Use of inventory schema has grown since its inception and we have are starting to ask for new attributes. That's why it should be specified

Copy link
Collaborator

Choose a reason for hiding this comment

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

@thearifismail their UUIDs aren't hyphenated, though, so I don't think they'd be able to use format: uuid.

Copy link
Contributor

Choose a reason for hiding this comment

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

Since a UUID without hyphens is a valid value, I thought it might pass the validation test. If not then should hyphenation be forced? @Tomboyo your thoughts?

Copy link
Author

Choose a reason for hiding this comment

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

I've gone back and spoken with the Hosted Candlepin team, and they would prefer that this schema treat the IDs as opaque strings (no format, no pattern) rather than try to codify the specific representation, which Candlepin considers to be an implementation detail. Does that sound okay?

items:
description: An environment UUID.
type: string
example: "262e621d10ae4475ab5732b39a9160b2, 01fd642e02de4e6da2da6172081a971e"
Copy link
Contributor

Choose a reason for hiding this comment

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

These examples need hyphens as required by the pattern specified.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The pattern does not specify hyphens; [0-9a-f]{32} means a 32 character string, where each character is a number or a letter from "a" to "f"

Copy link
Collaborator

@kruai kruai left a comment

Choose a reason for hiding this comment

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

Let's discuss the changes on this PR instead. The process is that after the inventory-schemas PR is merged, we open an insights-host-inventory PR with the automated changes from make update_schema.

@Tomboyo Tomboyo force-pushed the CHAINSAW-234-add-environment-schema branch from c1124c5 to fa1d7f3 Compare December 9, 2024 15:40
@Tomboyo
Copy link
Author

Tomboyo commented Dec 16, 2024

Per Candlepin maintainer feedback, I've updated the environment ID schema to be an opaque string, and removed any reference to it being a UUID in favor of just calling it an ID. I've also renamed rhsm.environments to rhsm.environment_ids so it's clear that the field contains identifiers. The same is done in the https://github.com/RedHatInsights/inventory-schemas/pull/145/files PR, so per Kruai we can continue conversation over there if that's preferred. Thanks all.

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.

3 participants