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

feat(remove): remove deprecated providerRef #475

Merged
merged 1 commit into from
Aug 23, 2023

Conversation

@haarchri haarchri requested review from a team as code owners July 4, 2023 11:13
@haarchri haarchri requested review from turkenh and ezgidemirel July 4, 2023 11:13
@haarchri haarchri force-pushed the feature/remove-dep-providerref branch from afcc517 to 3605cbb Compare July 4, 2023 11:37
@negz
Copy link
Member

negz commented Jul 5, 2023

AFAIK The only reason it's still there is that removing it would be a breaking API change.

I agree that it's been deprecated long enough that we should be fine to remove it - I'm not aware of any v1 APIs it would affect. Any providers that picked up the change would need to communicate it well.

@negz
Copy link
Member

negz commented Jul 5, 2023

Are there any other deprecated fields that we want to remove at the same time?

@haarchri
Copy link
Contributor Author

haarchri commented Jul 5, 2023

Have no other field in my mind -

@negz
Copy link
Member

negz commented Jul 6, 2023

@haarchri I had a look. There's a few others that are due for deprecation, e.g.:

  • spec.deletionPolicy (will be deprecated by spec.managementPolicies)
  • spec.writeConnectionSecretToRef (will be deprecated by spec.publishConnectionDetailsTo)

Given that we've waited this long to remove spec.providerRef I could be convinced to wait until management policies and secret stores are GA so we can remove all three deprecated fields at once. Feels like it might be better to batch up potentially breaking changes.

That said, I don't feel strongly. I suspect that removing spec.providerRef will not affect anyone at this point, unlike the other two examples.

Copy link
Member

@turkenh turkenh left a comment

Choose a reason for hiding this comment

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

While I like the idea of batching multiple at once, I believe the deprecation of providerRef could be much smoother compared to the other two as it is not prevalent as others, and it has been a long time since it was replaced with providerConfigRef.

So, I prefer to merge this and batch the other two together.

@haarchri haarchri force-pushed the feature/remove-dep-providerref branch from 3605cbb to 54aa59d Compare August 23, 2023 13:46
Signed-off-by: Christopher Haar <christopher.haar@upbound.io>
@haarchri haarchri force-pushed the feature/remove-dep-providerref branch from 54aa59d to fd85873 Compare August 23, 2023 13:47
@turkenh turkenh merged commit a597135 into crossplane:master Aug 23, 2023
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