Closed dteplits closed 1 year ago
General comment that I had to give on previous PRs (and can be fixed on a follow-up PR). I would change the code so there will be a clear API between ZoneFile, ZoneFileCache and Zonemanager. I mean, treat them as if they are in separate packages and there is no access to prived methods and data members. I believe it will make the code much more readable and safe.
Methods that I consider as a public API are: ZoneManager:
If I want to treat them as they are in a separate packages, I should make them public. It's not really necessary since it's still the same package but for the clarity of the code perhaps I should. Regarding the unit tests, I will change it to use public methods only.
Regarding the unit tests, I will change it to use public methods only.
Unit test can check private methods as well
Regarding the unit tests, I will change it to use public methods only.
Unit test can check private methods as well
Then I don't see a problem :) The only private method I use outside its file is ZoneFileCache.initSoaSerial() - in unit test Anyway, I can use a public one instead if necessary.
Regarding the unit tests, I will change it to use public methods only.
Unit test can check private methods as well
Then I don't see a problem :) The only private method I use outside its file is ZoneFileCache.initSoaSerial() - in unit test Anyway, I can use a public one instead if necessary.
The problem that Alona raise is the decoupling between the components IIUC unrelated to my specific comment about unit tests. Connection between components is done by the APIs.
Regarding the unit tests, I will change it to use public methods only.
Unit test can check private methods as well
Then I don't see a problem :) The only private method I use outside its file is ZoneFileCache.initSoaSerial() - in unit test Anyway, I can use a public one instead if necessary.
The problem that Alona raise is the decoupling between the components IIUC unrelated to my specific comment about unit tests. Connection between components is done by the APIs.
So based on the list I specified earlier (public API methods), converting those methods to public should do the job? This is an API as I define it. Do I miss something?
Regarding the unit tests, I will change it to use public methods only.
Unit test can check private methods as well
Then I don't see a problem :) The only private method I use outside its file is ZoneFileCache.initSoaSerial() - in unit test Anyway, I can use a public one instead if necessary.
The problem that Alona raise is the decoupling between the components IIUC unrelated to my specific comment about unit tests. Connection between components is done by the APIs.
So based on the list I specified earlier (public API methods), converting those methods to public should do the job? This is an API as I define it. Do I miss something?
Moving zone_file.go
and zone_file_cache.go
to package internal
should block accessing their public methods outside of the package - https://dave.cheney.net/2019/10/06/use-internal-packages-to-reduce-your-public-api-surface
Moving
zone_file.go
andzone_file_cache.go
to packageinternal
should block accessing their public methods outside of the package - https://dave.cheney.net/2019/10/06/use-internal-packages-to-reduce-your-public-api-surface
Imo we can just use Cap and non Cap draw 3 boxes, one for each module, and just the functions that have cross talk declare as API and expose. The internal can be the next step if needed, and then it might be easier because each module will also have its own folder (as the internal is based on folder structure)
Regarding the unit tests, I will change it to use public methods only.
Unit test can check private methods as well
Then I don't see a problem :) The only private method I use outside its file is ZoneFileCache.initSoaSerial() - in unit test Anyway, I can use a public one instead if necessary.
The problem that Alona raise is the decoupling between the components IIUC unrelated to my specific comment about unit tests. Connection between components is done by the APIs.
So based on the list I specified earlier (public API methods), converting those methods to public should do the job? This is an API as I define it. Do I miss something?
Moving
zone_file.go
andzone_file_cache.go
to packageinternal
should block accessing their public methods outside of the package - https://dave.cheney.net/2019/10/06/use-internal-packages-to-reduce-your-public-api-surface
will be fixed in a separate PR
/approve
/approve
[APPROVALNOTIFIER] This PR is APPROVED
This pull-request has been approved by: AlonaKaplan
The full list of commands accepted by this bot can be found here.
The pull request process is described here
I think we didn't cover the following case: Status-monitor container crashed, for example if someone removed kubevirt for more than 2 minutes and then deleted all the VMs Now reinstall kubevirt, and the container is starting with the previous SOA, but it doesnt write the file as long as there is no update, and the zone file still have the old VMs the safest solution is to rewrite the file (with serial++) without vms if the soa was there (but with the header and such, so we wont lose the serial in case of another restart)
Flow description:
PR adds the below logic:
If status monitor container was reloaded, on its startup it should check whether zone file already exist. If yes, it should continue the existing SOA serial sequence instead of setting it back to 1. The reason for that is the below edge case:
Release note: