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

Add provider name to provider resource #241

Merged
merged 7 commits into from
Jul 26, 2023

Conversation

jplewa
Copy link
Member

@jplewa jplewa commented Jul 10, 2023

Task

Resolves: #228

Description

Disclaimer: for providers with complex names (such as aws-native), the names have the following format: AzurenativeProviderResource. I'll try to figure out if there's an easy way to make the name AzureNativeProviderResource instead, but the easiest way was to ignore the capitalization of complex names.

TODO

  • Add tests Fix existing tests
  • If the current solution is determined to be not good enough, figure out a way to capitalize complex names

@jplewa jplewa self-assigned this Jul 10, 2023
@jplewa jplewa requested a review from a team as a code owner July 10, 2023 14:51
@jplewa jplewa linked an issue Jul 10, 2023 that may be closed by this pull request
@jplewa jplewa requested review from myhau and ddzikon July 10, 2023 14:51
@jplewa jplewa changed the title add provider name to provider resource Add provider name to provider resource Jul 10, 2023
@jplewa jplewa force-pushed the 228-rename-providerresource-to-provider-nameprovider branch 4 times, most recently from f5ea574 to 062cfee Compare July 13, 2023 10:22
@jplewa jplewa force-pushed the 228-rename-providerresource-to-provider-nameprovider branch from 062cfee to 28b17f6 Compare July 14, 2023 16:40
@jplewa jplewa requested a review from myhau July 14, 2023 16:40
@jplewa jplewa force-pushed the 228-rename-providerresource-to-provider-nameprovider branch from 28b17f6 to 3d03e16 Compare July 19, 2023 10:59
@jplewa jplewa force-pushed the 228-rename-providerresource-to-provider-nameprovider branch from 3d03e16 to 8e97914 Compare July 21, 2023 10:36
Copy link
Member

@myhau myhau left a comment

Choose a reason for hiding this comment

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

LGTM

I wish I could delve into this more, but two things that I noticed (partially related to this PR)

  • isProvider — passing this primitive argument through all the layers feels wrong to me. Maybe we should extend UsageKind by adding another Subject (Provider) or take a step back and think of a better design altogether?
  • PulumiName — I think we could refactor it and its usage a bit along with NamingConfiguration + UsageKind abstractions

@jplewa
Copy link
Member Author

jplewa commented Jul 26, 2023

I wish I could delve into this more, but two things that I noticed (partially related to this PR)

  • isProvider — passing this primitive argument through all the layers feels wrong to me. Maybe we should extend UsageKind by adding another Subject (Provider) or take a step back and think of a better design altogether?

  • PulumiName — I think we could refactor it and its usage a bit along with NamingConfiguration + UsageKind abstractions

Both good points. I tried to address the first one, but adding this new enum value isn't as straightforward as it seems (Subject isn't passed to PulumiName and I think we'd need to add an extra field in Modifiers which seems a bit clumsy). I'll create a task to address both of these things, but for now I'll merge it as is.

The task: #274

This was referenced Jul 26, 2023
@jplewa jplewa merged commit f0f99b4 into main Jul 26, 2023
@jplewa jplewa deleted the 228-rename-providerresource-to-provider-nameprovider branch July 26, 2023 09:17
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.

Rename providerResource to {provider-name}Provider
2 participants