-
Notifications
You must be signed in to change notification settings - Fork 319
DAOS-17659 control: Enable setting property value with multiple strings #16503
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
base: master
Are you sure you want to change the base?
Conversation
Features: control Signed-off-by: Tom Nabarro <thomas.nabarro@hpe.com>
Ticket title is 'Unable to set pool property |
Test stage Build RPM on EL 8 completed with status FAILURE. https://jenkins-3.daos.hpc.amslabs.hpecorp.net//job/daos-stack/job/daos/view/change-requests/job/PR-16503/1/execution/node/330/log |
Test stage Build RPM on EL 9 completed with status FAILURE. https://jenkins-3.daos.hpc.amslabs.hpecorp.net//job/daos-stack/job/daos/view/change-requests/job/PR-16503/1/execution/node/334/log |
Test stage Build RPM on Leap 15.5 completed with status FAILURE. https://jenkins-3.daos.hpc.amslabs.hpecorp.net//job/daos-stack/job/daos/view/change-requests/job/PR-16503/1/execution/node/327/log |
Test stage Unit Test on EL 8.8 completed with status UNSTABLE. https://jenkins-3.daos.hpc.amslabs.hpecorp.net/job/daos-stack/job/daos//view/change-requests/job/PR-16503/1/testReport/ |
@@ -159,6 +160,8 @@ func (f *SetPropertiesFlag) UnmarshalFlag(fv string) error { | |||
return propError("value must not be empty") | |||
} | |||
|
|||
value = strings.Trim(value, "\"") |
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.
value = strings.Trim(value, "\"") | |
value = strings.Trim(value, `"`) |
Thanks a lot for the quick responses, everyone. This patch doesn't feel right---hope I'm not misleading:
|
Yeah, after applying this patch, I got the same errors when setting
This worked, however:
Sounds like the |
If escaping single or double quotes isn't acceptable should the parser check if the option following the comma matches compatible flag name? try match keyval else try match flag? what do you think? |
Could we use some other set of characters to wrap the values (maybe I'm not completely against users having to escape the quotes, but they may find it a bit annoying (and hard to figure out what they did wrong if they don't escape them). |
Test stage Unit Test on EL 8.8 completed with status UNSTABLE. https://jenkins-3.daos.hpc.amslabs.hpecorp.net/job/daos-stack/job/daos//view/change-requests/job/PR-16503/2/testReport/ |
Test stage NLT on EL 8.8 completed with status UNSTABLE. https://jenkins-3.daos.hpc.amslabs.hpecorp.net/job/daos-stack/job/daos//view/change-requests/job/PR-16503/2/testReport/ |
Test stage Functional Hardware Medium MD on SSD completed with status FAILURE. https://jenkins-3.daos.hpc.amslabs.hpecorp.net//job/daos-stack/job/daos/view/change-requests/job/PR-16503/2/execution/node/1454/log |
Test stage Functional Hardware Medium Verbs Provider MD on SSD completed with status FAILURE. https://jenkins-3.daos.hpc.amslabs.hpecorp.net//job/daos-stack/job/daos/view/change-requests/job/PR-16503/2/execution/node/1544/log |
Fix issue where a property value containing multiple comma-separated
strings cannot be set e.g. self_heal:exclude,rebuild.
Features: control
Steps for the author:
After all prior steps are complete: