feat: add CLI commands for config policies [CM-423] #9911
Conversation
✅ Deploy Preview for determined-ui ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
salonig23
changed the title
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9911 +/- ##
==========================================
- Coverage 54.57% 54.57% -0.01%
==========================================
Files 1258 1259 +1
Lines 157138 157176 +38
Branches 3617 3618 +1
==========================================
+ Hits 85764 85775 +11
- Misses 71241 71268 +27
Partials 133 133
Flags with carried forward coverage won't be shown. Click here to find out more.
|
salonig23
force-pushed
the
add-constraints-CLI
branch
from
1a00a58
to
e038ba7
Compare
| @@ -163,6 +164,7 @@ def render_sequence(sequence: List[str]) -> str: | |||
| + sso.args_description | |||
| + oauth.args_description | |||
| + dev.args_description | |||
| + config_policies.args_description | |||
There was a problem hiding this comment.
I think we don't want to display help info for config policies until we're ready to release the feature, to prevent customers from accidentally discovering it. Is that possible?
There was a problem hiding this comment.
Do we want the command to still work even if it not displayed under help?
There was a problem hiding this comment.
+1 to Saloni's question! I was under the impression that these commands wouldn't work 🙈
@azhou-determined do you know if there's a way to "stub" out CLI commands without breaking the python linter?
There was a problem hiding this comment.
sorry i'm confused lol, what's going on? i'm sure i'm missing something, but if the commands won't work without other changes, shouldn't this just land in a feature branch?
There was a problem hiding this comment.
The commands will work to varying degree. The APIs are all stubbed at the moment, so the commands will function but not in a customer-ready way.
My thought was that, if we added the CLI commands in a non-discoverable way that we could use them to enable testing while we continue to develop features. @salonig23 a feature branch is also ok; I was trying to enable us to merge to main as much as possible for convenience / developer velocity.
There was a problem hiding this comment.
I will wait to merge this PR after RC cut for the next release - most of the API functions should be implemented by then and I can also work on switching the tests to unit tests intermittently
| "--workspace-name", | ||
| type=str, | ||
| required=True, # change to false when adding --global | ||
| help="config policies for the given workspace", |
There was a problem hiding this comment.
| help="config policies for the given workspace", | |
| help="the workspace whose config policies are being deleted ", |
There was a problem hiding this comment.
I actually prefer config policies for the given workspace but I think either is fine.
There was a problem hiding this comment.
Shouldn't the argument's help description correspond to what it is though?
There was a problem hiding this comment.
i'd say since there is a separate help string for the command itself, we don't have to customize the help strings based on which command it is in. But maybe @azhou-determined can weigh in
| "--workload-type", | ||
| type=str, | ||
| required=True, | ||
| help="config policies for a given workload, options: Experiment and NTSC", |
There was a problem hiding this comment.
| help="config policies for a given workload, options: Experiment and NTSC", | |
| help="the type (Experiment or NTSC ) of config policies to set", |
There was a problem hiding this comment.
I don't think we necessarily have to include the command in the description for these arguments, because there is a separate help string for the command. For example, all the workload-type help strings can be the same: "the type (Experiment or NTSC ) of config policies" Wdyt?
There was a problem hiding this comment.
probably a bit too late for this since you already wrote them, but we've been trying to get CLIs out of e2e tests and into unit tests. generally speaking, our e2e tests are expensive to run and maintain, and usually more trouble than they're worth. we haven't always been thoughtful about what kinds of tests we consider "e2e" tests, and we pay the price when we try and change existing features/write new code in the future.
ideally, we'd have unit tests for these cases (see /harness/tests/cli/) that just mock the bindings call and verify that the correct values are passed in/gotten out. most CLI commands are simply argument parsing + calling bindings and shouldn't warrant a full on integration test.
it'd be great if we could turn these into unit tests, but it's not a big enough deal that i'd consider it a blocker. i like how you structured them and which cases to test, and i can tell you put a lot of work into these! if you need to land this quickly, i understand. i'll figure out a better way to communicate this kind of stuff across teams in the future, before PR time 🙃
There was a problem hiding this comment.
Makes sense, I will be changing the tests to unit tests. Thanks for the thorough feedback!
| @@ -163,6 +164,7 @@ def render_sequence(sequence: List[str]) -> str: | |||
| + sso.args_description | |||
| + oauth.args_description | |||
| + dev.args_description | |||
| + config_policies.args_description | |||
There was a problem hiding this comment.
sorry i'm confused lol, what's going on? i'm sure i'm missing something, but if the commands won't work without other changes, shouldn't this just land in a feature branch?
|
|
||
| args_description: cli.ArgsDescription = [ | ||
| cli.Cmd( | ||
| "config-policies", |
There was a problem hiding this comment.
how about just calling this "policies"? i'd imagined that "policies" would basically mean "constraint configs", and "config-policies" is a lot to type
There was a problem hiding this comment.
@kkunapuli @maxrussell what do you think about just policies? I remember deciding on config-policies to make it more discoverable and clear but I am open to either
There was a problem hiding this comment.
I think we should use "task config policies" in documentation as much as possible, but I'm fine with using just policies for the CLI command.
There was a problem hiding this comment.
I have two concerns with just "policy":
- I'm concerned that we'll have more than one kind of policy in our system and this will be convenient shorthand for now and then become confusing and ambiguous later.
- I'm also concerned that without a noun or verb to attach to "policy," users won't know what it's for and we'll have the discoverability problem Saloni mentioned. Log retention policy? Garbage collection policy? etc.
There was a problem hiding this comment.
that's fair.
i was thinking about "policies" as a feature that would grow into something generic enough to encompass all sorts of system policies, or at least be the main "policy" feature that we support that it wouldn't be ambiguous.
if all policies are attached to a config, in the future there might exist some parallel between existing CLI commands and their policies, like det e set log-retention 1 and det e policies set log-retention 1. though the abstraction might not be as clean as i'm imagining.
i agree thought that right now just "policies" is kind of vague.
There was a problem hiding this comment.
That's a cool idea! Interesting to have it be kind of a subtree of the CLI that mirrors others.
salonig23
force-pushed
the
add-constraints-CLI
branch
from
eabae88
to
6d9223b
Compare
salonig23
force-pushed
the
add-constraints-CLI
branch
from
6d9223b
to
e075ff1
Compare
Ticket
CM-423
Description
Add CLI commands and e2e tests for config policies.
Test Plan
Automated tests added.
Checklist
docs/release-notes/See Release Note for details.