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
server: don't export B&R APIs if feature is not enabled globally #4202
Conversation
This change will ensure that B&R APIs are not exported if the feature is not enabled in any of the zones. Signed-off-by: Rohit Yadav <rohit.yadav@shapeblue.com>
@blueorangutan package |
@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
Packaging result: ✔centos7 ✔debian. JID-1534 |
@blueorangutan test |
@rhtyd a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests |
Trillian test result (tid-1994)
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR @rhtyd, code LGTM.
I just left a minor remark; could you please check if the raised question makes sense?
@@ -38,7 +38,7 @@ | |||
ConfigKey<Boolean> BackupFrameworkEnabled = new ConfigKey<>("Advanced", Boolean.class, | |||
"backup.framework.enabled", | |||
"false", | |||
"Is backup and recovery framework enabled.", true, ConfigKey.Scope.Zone); | |||
"Is backup and recovery framework enabled. Restart management server on global value change.", true, ConfigKey.Scope.Zone); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to the description, ADMIN needs to restart the management server and therefore this is not a dynamic setting.
What do you think of changing the isDynamic
param to false
?
ConfigKey constructor:
public ConfigKey(String category, Class<T> type, String name, String defaultValue, String description, boolean isDynamic, Scope scope) {
this(type, name, category, defaultValue, description, isDynamic, scope, null);
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll change that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed thanks
Signed-off-by: Rohit Yadav <rohit.yadav@shapeblue.com>
Signed-off-by: Rohit Yadav <rohit.yadav@shapeblue.com>
@blueorangutan package |
@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
Packaging result: ✔centos7 ✔debian. JID-1536 |
@blueorangutan test |
@rhtyd a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests |
@davidjumani please review this, thnx |
Trillian test result (tid-2008)
|
@blueorangutan package |
@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
Packaging result: ✔centos7 ✔debian. JID-1539 |
@blueorangutan test |
@rhtyd a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests |
Packaging result: ✔centos7 ✔debian. JID-1547 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Test passes for simulator
Tested on KVM env, it skips tests for dummy provider now:
|
…che#4202) This change will ensure that B&R APIs are not exported if the feature is not enabled in any of the zones. Signed-off-by: Rohit Yadav <rohit.yadav@shapeblue.com>
This change will ensure that B&R APIs are not exported if the feature is not enabled globally. Make it non-dynamic to require restarting of mgmt server which is needed for the background sync task to be setup or not.
Types of changes