-
Notifications
You must be signed in to change notification settings - Fork 35
Send deprecation warnings for schema API #7424
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
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (3)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
CodSpeed Performance ReportMerging #7424 will not alter performanceComparing Summary
|
39a6fc3 to
a2e995a
Compare
|
Great improvement Also I don't think we should assume that a warning will always be associated with a single kind, I'm sure we'll find situations where a warning needs to be associated with multiple kinds or none at all. it would be good to ensure that the structure will support that. |
Yes that was the part I wasn't sure about from the description. I don't see any situation just now what kind of error it would be that wouldn't include a kind. Might be that |
a2e995a to
c7933ec
Compare
|
|
||
| class SchemaWarningKind(BaseModel): | ||
| kind: str = Field(..., description="The kind impacted by the warning") | ||
| attribute: str | None = Field(default=None, description="The attribute impacted by the warning") |
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.
I would replace attribute with field since it could also be a relationship.
We are using field or element in a few places to represent either an attribute or a relationship
92abff5 to
40281e5
Compare
40281e5 to
fd08dc5
Compare
This PR adds the concept of returning deprecation warnings through the API endpoints when loading or checking a schema. The idea is to help users to determine when they are using deprecated features.
It will go together with this PR in the SDK repo: opsmill/infrahub-sdk-python#582
The change is quite simple, one thing I'm not completely sure of is the structure of the warning that currently looks like this:class SchemaWarningType(Enum):
DEPRECATION = "deprecation"
class SchemaWarning(BaseModel):
type: SchemaWarningType = Field(..., description="The type of warning")
kind: str = Field(..., description="The kind impacted by the warning")
message: str = Field(..., description="The message that describes the warning")
I'm thinking that later we could add other warning types such asSTRICTfor when Infrahub isn't running in strict mode and we want to report back on those issues.What I'm unsure of is thekind: strif we think that any future schema warning will always be tied to a kind, just now I can't think of other variations but it could just be that I'm missing it now. Any thoughts? We can always modify it later, but that might be a breaking change. I think it's fine as it is now, but not certain..