updates to make module more production ready#161
updates to make module more production ready#161kewalaka wants to merge 49 commits intoAzure:mainfrom
Conversation
|
if you do find the useful, I would suggest we land it in a different branch, not main, so we can do some isolated testing. for my scenario, this is working well, but I haven't regression tested it. i'm also aware of the resource module being worked on too, thus I didn't want ot waste time on this if the ptn module is going to get refactored to re-use this new resource module. |
|
if this is of general use, but the monitoring adds to much noise, i'm happy to split that out. However if it's not in the direction you want to go, I'd rather you say so, please. |
|
I note this branch now has write conflicts that weren't there when I raised the PR. As I mentioned above, happy to split out the monitoring piece if the general approach abov is of interest, but please say if it is not. |
|
@kewalaka Thank you for your contribution. It might take us a while to review this, so please bear with us. |
|
@nellyk i would suggest the initial thing is you decide whether you like the idea of the monitoring being included. Then if you like I can split this PR to make a smaller changeset. Like I said, I'm happy to do more work to make it digestible, but if you generally don't agree with the extra features added then we will have to part ways. I don't mind (tho would like to get this on AVM), just would like your opinions, not necessarily a deep technical review at this stage. |
|
Any eta? need part of ingress profile |
|
@kewalaka thanks for contributing this awesome pr to us, unfortunately @nellyk could not maintain this module so I'd like to take from here. Since this pr looks a huge one and to be honest I am not an expert in Aks, it might take a while for me to go through your pr, and even I will open multiple small prs instead of merging this one, but let me be clear, I won't be able to do all of these without your help and you should take the credit. |
|
hi @lonegunmanb - humble apologies for the large PR, I haven't had chance to break it down. I was thinking that this module might get refactored using the new resource-based module, which is another reason why I didn't pursue it. That, and if I was to invest further time into AKS, my personal preference would be a base built on AzAPI. My original starting point with this was to mimic the capability of AKS Automatic, that's where most of the technical decisions originate, whilst optionally allowing users to bring their own monitoring components if they already had them made. I'm not sure I'll be much help, but happy to answer any questions if I can. |
Description
@nellyk in response to your question, whether I have done any work on this.
I haven't had chance to break this down into multiple PRs, but we ran into a number of issues using this module, that this change addresses. I realise some are cosmetic. I also realise its a large PR.
Change highlights
In order of changes below:
private_dns_zone_id_enabledis a bit misleading and has been renamed toprivate_dns_zone_set_rbac_permissionswhich is actually what it does.nameshould be used rather thankubernetes_nameterraform_datais the generally preferred replacement for your use of the null providerThe above lints & changes currently in main have been merge into it.
closing notes
I don't usually like to PR changes this large, unfortunately time constraints have meant I haven't had chance to do this in stages.
That, and I suspect some of the above you may not agree with.
I understand you're wanting to make some decisions for the caller, but in our use of this we found it too restrictive to be useful, and I propose that are requirements are not going to be especially unusual.
Happy for discussions....
Type of Change
Checklist