Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
refactor!: rename Container.exec_mocks to Container.execs and extend mocking #124
refactor!: rename Container.exec_mocks to Container.execs and extend mocking #124
Changes from 34 commits
73eba4f
63b62c8
830c732
e84a1c3
e827b92
01ae7ac
9077a53
d39cfd8
2d0734a
514c37f
dffea4d
53f2296
f1fe0d1
e86d703
5e9ab73
fb75c86
52f517c
36327d6
d3f9916
38c7a71
dbfbb91
3492ab1
0cad14c
3e4c574
03ef892
432f50a
02d59be
4221b02
e3a15bb
cc8b3ca
df5462f
7fe631d
ee72367
a631927
23241cb
42f21b4
15ea896
8928a0f
acdb14f
71525f0
5000836
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
not sure I understand what's going on.
Suppose you have two execs, one for
ls -ll
and one forls -la
, and the charm doesls --fubar
. Which exec mock will this match?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 think that neither will match.
However, if the real command is
ls -la -ht
, thenls -la
will match.Did I get that right?
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.
The Harness behaviour (which I intended to match here, although I think I'm too low on tests) is that the longest match wins, but also that you have to match what's there. So in your example, neither would match.
I'm not sure what Harness does if there are two that have the exact same command pattern, but it seems to me that should be an error (consistency check?).
What I see charms doing (with Harness) is:
[]
, presumably because they know that only one command will run and they either want to keep the test code clean or can't be bothered copying the exact command over to the test code[first]
when the command is things like[first, arg1, arg2, arg2]
- probably partly the same reasoning as[]
but maybe also because there are multiple ways that the charm could run it and the result should always be the same (I haven't looked deeply enough to check)It does seem like having this is a nice convenience. @benhoyt probably has the full story on how Harness came to have that behaviour (it slightly predates me joining Canonical if I have the timing right).
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.
Talked over this with @benhoyt on a call also, and he agrees that it's worth keeping the prefix matching in. The spec for adding exec testing in Harness has some more of the background/details.
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.
in order to avoid confusions, how about we make this private and add a
stdin
property?Also, conceptually, I feel that
stdin
does not quite belong to the State but to Context and is more on the level ofjuju_log
.Just spitballing here, but how do we feel about:
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.
Yes, that's much better, thanks! Done. (Although it may end up being irrelevant anyway!)
I feel like it's much tidier to have everything bundled up together in the one mock process object rather than split off the input to it into the context.
In addition, it does seem reasonably likely that we'd want to extend this in the future to handle the other 'input' elements of
exec()
- to verify that the right envars are being set, or that it's being run in the right working directory, or as the right user, and so on. It would be fairly clean to add those as additional private attributes ofExec
and properties to expose them. I don't think we would want all of them as additional context methods, or to havectx.get_exec_input(container, command)
return some new object (essentiallyops.testing.ExecArgs
) that collects them all.I also don't love how the
Context
object is becoming more and more context+side_effects.I do get that it's not in the output state in the way that changes to secrets or relation data are - once the process has finished, it's gone. It somewhat feels like stdout/stderr/return_code are the same, in that the executable presumably still lives in the container but what happens when you (mock) run it isn't really "state".
I spent a few minutes convincing the rest of charm-tech that it should stay as it is in our daily call today, and then as I've tried to write it out I've found myself disagreeing with my own arguments and trying to come up with alternatives (I will spare you the terrible ones!).
An executable is basically a method that takes some input and returns some output. What if we did something similar to the
handle_exec
system from Harness instead?So the mock executable lives on in the state, but the input and output is something that's is re-run each time. The input, output, and exit code all live together in one (temporary) place.
I'm not a huge fan of having some of the asserts end up in the mock function, breaking the very clean typical pattern of arrange/act/assert. However, you could still do that with
nonlocal
or like:I would personally leave out the "results" shortcut that Harness has. It could always be added later, and it's simple to do -
result=127
is justlambda _: ExecResult(exit_code=127)
andresult="foo"
is justlambda _: ExecResult(stdout="foo")
.WDYT? Also @benhoyt since we were in agreement on a different result earlier today, sorry! I probably should have written up my draft response before talking to the team and then would have come to a different conclusion earlier.
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.
mmm I don't really like the proposal of doing the asserts in a temporary spot. It's very un-scenario-y.
perhaps:
with the risk that if you need to mock lots of commands you end up with a confusing bunch of individual contexts?
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 do also like the flexibility that a method has, for things like returning different output depending on the arguments or the stdin. On the other hand, it can't be included in a JSON dump like the rest of the
State
can, which is not great - although arguably it's not that different fromContainer.mounts
serialising to a map of locations of content, not the content itself.I played around with contexts as well - they do seem to be a reasonably natural fit in terms of having something that only exists within a specific scope. It can indeed end up with a lot, e.g.:
It's worse in Python <3.10 where you don't get to wrap the contexts in
()
.What about bundling it into the existing context manager?
If you don't care about how the process is executed and just need to mock out the output/return code then you don't need to use the context manager. I do like it more when all the pieces of the process are together, but I also get that it's potentially confusing with some being input and some output (and it's kinda opposite to what you expect, since the input the charm writes is the output of the test, etc).
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.
mmm, quite like the idea of enclosing it in the Manager object, but I don't think that's going to be as easy to remember as having it directly on the exec.
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.
in terms of API discoverability that's a net loss I'm afraid
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.
Ok, so I've taken this back to something much closer to @PietroPasotti's earlier suggestion, as also discussed with the Charm-Tech team last Friday.
The
Exec
object only has the results of the process (exit code, stdout, stderr), just like in Scenario 6, and the input to the process (stdin, user, group, environment, etc) are available from the Context in a newexec_history
attribute.I feel like we're doing a great job of keeping the
State
only state but not a great job at keepingContext
only context - it's more and more "context plus side effects". But this way of handling exec is consistent at the moment, so let's go with this way, and I'll open a different ticket for tidying upContext
.