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

Extend infer with annotations for aliases and resource deprecations #245

Merged
merged 2 commits into from
Jun 21, 2024

Conversation

thomas11
Copy link
Contributor

This allows authors of providers built on infer to alias and deprecate resources.

@thomas11 thomas11 requested review from danielrbradley, iwahbe and a team June 21, 2024 10:10
@thomas11 thomas11 changed the title Extend infer with annotations for aliases and resources deprecations Extend infer with annotations for aliases and resource deprecations Jun 21, 2024
Copy link

@flostadler flostadler left a comment

Choose a reason for hiding this comment

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

Nice!

@thomas11 thomas11 merged commit b7d8dc6 into main Jun 21, 2024
6 checks passed
@thomas11 thomas11 deleted the tkappler/aliases branch June 21, 2024 10:18

AddAlias(module, name string)

SetResourceDeprecationMessage(message string)
Copy link

Choose a reason for hiding this comment

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

Nitpicking, but given that this is backed in the implementation by a field called DeprecationMessage, I'm inclined to think Resource is superfluous here? Moreover, if we're feeling cheeky, we could call it Deprecate to align with Describe.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, good thoughts .. perhaps also AddAlias could just be Alias?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I called it SetResourceDeprecationMessage because resource properties can also be deprecated. The field DeprecationMessage is on the resource level so there's no ambiguity.

For the aliases, I went with "add" because it can be called more than once to add different aliases, which I wanted to call attention to.

Let me know if my reasoning doesn't make sense to you.

Copy link
Member

Choose a reason for hiding this comment

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

Describe also applies to both the resource and it's fields. I agree that Deprecate would have been a more consistent name.

thomas11 added a commit that referenced this pull request Jun 21, 2024
iwahbe added a commit that referenced this pull request Jun 22, 2024
Following up on #245, we specify the boundary requirements of `AddAlias` and `SetToken`
with the types that they accept. This is a breaking change unless you are using untyped
constants in `SetToken` and `AddAlias`, which I expected to be the common case.
iwahbe added a commit that referenced this pull request Jun 23, 2024
Following up on #245, we specify the boundary requirements of `AddAlias`
and `SetToken` with the types that they accept. This is a breaking
change unless you are using untyped constants in `SetToken` and
`AddAlias`, which I expected to be the common case.
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.

5 participants