-
-
Notifications
You must be signed in to change notification settings - Fork 513
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
Make mutexes private #890
base: master
Are you sure you want to change the base?
Make mutexes private #890
Conversation
Hi @154pinkchairs , Thank you for these improvements! Making the mutexes private and the FirewallError type would be really nice to have. Could you split the PR into smaller ones? "Make mutexes private" has nothing to do with adding a new error type, it'll make things easier to analyze and debug. On the other hand, could you add only the changes relevant to the PR? Testing it out: Also, the new json tags doesn't correspond with the existent system-fw.json configuration fields, which on the one hand would break users' configurations, and on the other hand prevents from adding system firewall rules. For example renaming "Version" to "versions" prevents from adding rules: Other examples: And here another change not relevant to the PR: |
…or type" This reverts commit 6ef14d5.
chore(config): remove errantly removed comments and tags
thank you @154pinkchairs , looks better now! There's only one minor problem, that's probably why these mutexes were embedded instead of being members of the structs.
I've only found 2 ways of getting rid of it: 1) by embedding the lock as it was before, 2) by creating a new SystemConfig struct:
But unfortunately, doing it in this way, it does not delete all the rules when changing the policies. |
see: uber-go/guide#127
in terms of stylistic changes this also combines the IPv4 and IPv6 errors in an unified struct.