Feature #15467
closed
Revisit new config, ordered lists must end in "List" and unordered sets should be turned into maps
100%
Description
In Cluster configuration we say "Arrays and lists are not to be used" (and recently added "unless order is truly significant.")
But we already added some new config keys that broke this rule, including:- DriverParameters.SecurityGroupIDs
- DisabledAPIs
- NewUserNotificationRecipients
- NewInactiveUserNotificationRecipients
These should be changed from lists to maps. For example, instead of
SecurityGroupIDs: - a - b - c
we should have
SecurityGroupIDs:
a: {}
b: {}
c: {}
We can maintain compatibility with old configs the way we did for InstanceTypes: if the JSON value starts with '[', unmarshal into a slice of strings and build an appropriate map. See (*InstanceTypeMap)UnmarshalJSON() in source:sdk/go/arvados/config.go.
We also need a test in lib/config that- prohibits list/array values on keys that don't have the suffix "List".
- prohibits the suffix "List" on keys whose values aren't lists/arrays.
Related issues
Updated by Peter Amstutz almost 4 years ago
- Status changed from New to In Progress
Updated by Peter Amstutz almost 4 years ago
- Status changed from In Progress to New
Updated by
Updated by Peter Amstutz almost 4 years ago
- Target version changed from 2019-07-17 Sprint to 2019-07-31 Sprint
Updated by Tom Morris
Updated by Peter Amstutz almost 4 years ago
- Status changed from New to In Progress
Updated by Peter Amstutz almost 4 years ago
Updated by Peter Amstutz almost 4 years ago
15467-config-fixups @ 46d2d8aa38c07675081266bc287e4fb1f7149bb0
Added "StringSet" which is a map[string]struct{} that will parse a json list for backwards compatability.
https://ci.curoverse.com/view/Developer/job/developer-run-tests/1434/
Updated by Eric Biagiotti almost 4 years ago
Peter Amstutz wrote:
15467-config-fixups @ 46d2d8aa38c07675081266bc287e4fb1f7149bb0
Added "StringSet" which is a map[string]struct{} that will parse a json list for backwards compatability.
https://ci.curoverse.com/view/Developer/job/developer-run-tests/1434/
In lib/config/load_test.go, TestListKeys:
- I think this test needs cases where YAML is loaded and the errors are actually raised. Seems like testing a test, but this is really default config validation code that doesn't have coverage.
- ln 402: It's difficult to follow all the reflection here. Maybe a comment or some well-named variables would help.
Updated by Peter Amstutz almost 4 years ago
Eric Biagiotti wrote:
Peter Amstutz wrote:
15467-config-fixups @ 46d2d8aa38c07675081266bc287e4fb1f7149bb0
Added "StringSet" which is a map[string]struct{} that will parse a json list for backwards compatability.
https://ci.curoverse.com/view/Developer/job/developer-run-tests/1434/
In lib/config/load_test.go, TestListKeys:
- I think this test needs cases where YAML is loaded and the errors are actually raised. Seems like testing a test, but this is really default config validation code that doesn't have coverage.
Ok, I added a couple of synthetic cases.
- ln 402: It's difficult to follow all the reflection here. Maybe a comment or some well-named variables would help.
Cleaned up a bit.
15467-config-fixups @ 1bc9d9cb0dac110676f9fd60b2fc128d54b1abf5
https://ci.curoverse.com/view/Developer/job/developer-run-tests/1437/
Updated by Eric Biagiotti almost 4 years ago
Peter Amstutz wrote:
Ok, I added a couple of synthetic cases.
It's missing the case where it ends in "List" but isn't a list, but other than that it LGTM if the tests pass. Thanks!
Updated by Peter Amstutz almost 4 years ago
- Target version changed from 2019-07-31 Sprint to 2019-08-14 Sprint
Updated by Peter Amstutz almost 4 years ago
- Has duplicate Story #15147: Replace lists of strings in new config with maps added
Updated by Peter Amstutz almost 4 years ago
15467-legacy-config @ 6c89457a8f144d03f230656a1f4c43675d066b8c
Workbench and API server use -skip-legacy to suppress loading legacy config files of Go components (that they didn't load before).
Go components specify which legacy config file to use as argument to MungeLegacyConfigArgs, disable loading other config files.
https://ci.curoverse.com/view/Developer/job/developer-run-tests/1442/
Updated by Eric Biagiotti almost 4 years ago
Peter Amstutz wrote:
15467-legacy-config @ 6c89457a8f144d03f230656a1f4c43675d066b8c
This LGTM, thanks!
Updated by Eric Biagiotti almost 4 years ago
Reviewing 0ea1d6435771e00c7b84b50dc0cff1a13a1d7b67
- In install-dispatch.html, under Containers.ReserveExtraRAM, the introductory sentence says the value will be in bytes, but the example shows an arvados.ByteSize value. Maybe describe the units like we do for PollInterval?
- The "00000-bi6l4-000000000000000" used in the Containers.SLURM.KeepServices documentation is a bit confusing. I could imagine a user seeing that and thinking "Does my KeepServices have a different UUID, am I supposed to replace this?".
Updated by Peter Amstutz almost 4 years ago
Eric Biagiotti wrote:
Reviewing 0ea1d6435771e00c7b84b50dc0cff1a13a1d7b67
- In install-dispatch.html, under Containers.ReserveExtraRAM, the introductory sentence says the value will be in bytes, but the example shows an arvados.ByteSize value. Maybe describe the units like we do for PollInterval?
Added documentation about suffixes.
- The "00000-bi6l4-000000000000000" used in the Containers.SLURM.KeepServices documentation is a bit confusing. I could imagine a user seeing that and thinking "Does my KeepServices have a different UUID, am I supposed to replace this?".
Changed over to SbatchEnvironmentVariables as discussed on chat.
15467-update-install-docs @ a5e9c92d5d9e3a5575f9a103723d1814f5a51b66
Updated by Eric Biagiotti almost 4 years ago
Looks like lib/dispatchcloud tests failed.
Also, the code snippet under Containers.SLURM.SbatchEnvironmentVariables needs to be updated from "http://127.0.0.1:25107": {} to "ARVADOS_KEEP_SERVICES": "https://example.com/keep1 https://example.com/keep2" or whatever example makes sense.
Updated by Peter Amstutz almost 4 years ago
Eric Biagiotti wrote:
Looks like lib/dispatchcloud tests failed.
I suspect that is just a flaky test, but I submitted a new run:
https://ci.curoverse.com/job/developer-run-tests/1446/
Also, the code snippet under
Containers.SLURM.SbatchEnvironmentVariablesneeds to be updated from"http://127.0.0.1:25107": {}to"ARVADOS_KEEP_SERVICES": "https://example.com/keep1 https://example.com/keep2"or whatever example makes sense.
Fixed
15467-update-install-docs @ 5dbee91f63a39f19b1b8a63f0fda0f7115e7356e
Updated by Eric Biagiotti almost 4 years ago
Peter Amstutz wrote:
Eric Biagiotti wrote:
Looks like lib/dispatchcloud tests failed.
I suspect that is just a flaky test, but I submitted a new run:
https://ci.curoverse.com/job/developer-run-tests/1446/
Also, the code snippet under
Containers.SLURM.SbatchEnvironmentVariablesneeds to be updated from"http://127.0.0.1:25107": {}to"ARVADOS_KEEP_SERVICES": "https://example.com/keep1 https://example.com/keep2"or whatever example makes sense.Fixed
15467-update-install-docs @ 5dbee91f63a39f19b1b8a63f0fda0f7115e7356e
This LGTM, thanks!
Updated by Peter Amstutz almost 4 years ago
- Status changed from In Progress to Resolved