search

Kuadrant / architecture

Architecture Documents
0 stars 10 forks source link

rfc for removal of managed zone API #93

Open maleck13 opened 2 months ago

maleck13 commented 2 months ago

@jsmolar @mikenairn @philbrookes

fixes #67

maleck13 commented 2 months ago

My biggest problem with this RFC is that it is bundling together two things that are not neccessarily connected in my eyes:

  1. Removal of the ManagedZoned and replacing the content of credential secret
  2. Adding ability to have multiple credentials for DNSPolicy

Sure I can see that. But we have to remove the managedzone ref as part of this work and replace it with a secretRef anyway, so it seemed a reasonable to question if we should go with a single secret or allow multiple as part of this work allowing us to remove a limitation the DNSPolicy has currently and avoid two breaking changes one after the other. So this was really an attempt to be somewhat efficient and remove the need for another RFC.

pehala commented 2 months ago

Sure I can see that. But we have to remove the managedzone ref as part of this work and replace it with a secretRef anyway, so it seemed a reasonable to question if we should go with a single secret or allow multiple as part of this work allowing us to remove a limitation the DNSPolicy has currently and avoid two breaking changes one after the other. So this was really an attempt to be somewhat efficient and remove the need for another RFC.

Sure, my problem with this is that while removing managedzone is simple enough and probably will go through without much debate. Multiple credentials is a new feature and might require more discussion. When I looked through the PRs I didnt pay much attention to this RFC because I thought it handles just the removal.

I would either split this into two RFC or rescope this RFC to focus mainly on the new feature and as part of that we will remove ManagedZone

maleck13 commented 2 months ago

Sure I can see that. But we have to remove the managedzone ref as part of this work and replace it with a secretRef anyway, so it seemed a reasonable to question if we should go with a single secret or allow multiple as part of this work allowing us to remove a limitation the DNSPolicy has currently and avoid two breaking changes one after the other. So this was really an attempt to be somewhat efficient and remove the need for another RFC.

Sure, my problem with this is that while removing managedzone is simple enough and probably will go through without much debate. Multiple credentials is a new feature and might require more discussion. When I looked through the PRs I didnt pay much attention to this RFC because I thought it handles just the removal.

I would either split this into two RFC or rescope this RFC to focus mainly on the new feature and as part of that we will remove ManagedZone

Yeah I think that is reasonable. And to be fair the concept emerged as part of thinking about removal. So I guess we can say the original RFC was intended to just be removal of the MZ but our thinking evolved and so re-focusing the RFC to be around supporting multiple providers and domains from a single policy is a reasonable approach or removing that part into a separate RFC 👍

maleck13 commented 1 month ago

@mikenairn could I get a review on this