Add session ops utility with some facilitator functions (v2)

[pevogam]

Add a small library of frequent operations performed in aexpect sessions.

v1: Initial pull request, later split into manageable parts.
v2: Main session ops module contribution and selection of reusable session functions, rebased on most recent aexpect tag.

[pevogam]

The CI problem detected here is not related to these changes. Not sure how it wasn't detected in earlier pull requests but I will push one to fix it next.

[pevogam]

All CI problems fixed and PR can now be fully reviewed.

[ldoktor]

Hello @pevogam, I gave it another try after this long time and my feeling is still the same. Although it simplifies certain workflows I don't think it fits aexpect library. I can imagine it as another library that would generate command strings that would be then consumed by aexpect (or other library that allows shell-like interface).

My main problem is the size, the higher-level aim with security checks that are present in many places but in another they are missing (eg. stat(a, "/tmp/a", fmt="%; echo this could as well be formatting your hard drive; ").

I can create a repo for you and make you an admin of that, if you are interested, but I don't like the idea of having this in the core aexpect (note ideally I'd extract and publish the remote.py separately as well, but it's already there...)

Anyway @clebergnu, @lmr please take a look at this and if you decide it belongs here, feel free to give it a try.

[ldoktor]

Hello @pevogam, I gave it another try after this long time and my feeling is still the same. Although it simplifies certain workflows I don't think it fits aexpect library. I can imagine it as another library that would generate command strings that would be then consumed by aexpect (or other library that allows shell-like interface).

By this I mean something like:

import aexpect
from aexpect_ops import *

session = aexpect.ShellSession("sh")
session.cmd(stat("/tmp/a"))
session.cmd(copy("/tmp/a", "/tmp/b"))
...

(and I know it wouldn't allow the same flexibility, but should suffice the usual use cases)

[pevogam]

Hi @ldoktor,

Hello @pevogam, I gave it another try after this long time and my feeling is still the same. Although it simplifies certain workflows I don't think it fits aexpect library. I can imagine it as another library that would generate command strings that would be then consumed by aexpect (or other library that allows shell-like interface).

You might be able to conclude that from the current selection of commands but there are commands that would require only sendline, some would require sending special characters like ^C or ^D, others would require varied timeouts and arguments to the session.cmd calls or different session.cmd_* calls. So the current one-string-preparation-for-session-command approach would reduce a few of the simplest commands but will not generalize over future ones.

My main problem is the size, the higher-level aim with security checks that are present in many places but in another they are missing (eg. stat(a, "/tmp/a", fmt="%; echo this could as well be formatting your hard drive; ").

For sure it might have some limitations that we all expect to be pointed out during review and patched up before merging. I think you could notice that this pull request combines lots of different code from different authors in our team and previously added in different times when they were actually needed. So some unification is definitely to ask but I don't think will be hard to accomplish and this is what reviews are for - to discover maintainers' minimum uniform interface requirements (and leave some divergence that will be unavoidable aside).

I can create a repo for you and make you an admin of that, if you are interested, but I don't like the idea of having this in the core aexpect (note ideally I'd extract and publish the remote.py separately as well, but it's already there...)

Why would the remote be considered separate from aexpect? The entire point of our previous arguments in the past was that it makes more sense in aexpect than in Avocado VT and I think it extends the client module quite nicely adding the file transfer and session login/prompts support. The same can now be said about the remote_door module which extends this further with remote function execution and remote object method calls. But letting all of this aside, do you think that a separate project would make things easier? One thing we seem to have learned from the past is that fragmenting projects into too many small ones adds a lot of deployment, versioning, and maintenance overhead resulting in hours of packaging, debugging and testing of version compatibility and so on.

Anyway @clebergnu, @lmr please take a look at this and if you decide it belongs here, feel free to give it a try.

Another point I want to add here is that we find lots of contributors on the Avocado and Avocado VT side reimplementing a lot of these functions on top of a session again and again which confirms the need to have single reusable source that everybody could instead simply spend their time extending and saving the rest of their costs. As these utilities depend only on aexpect and all they require is a session I don't think it makes sense to keep them in Avocado or Avocado VT repos whcih specialize in something else.

Regarding a separate aexpect repo, can you elaborate more on what are the benefits from keeping current aexpect to the minimal code base and functionality it currently has? I would assume reduced maintenance cost and contributor activity is one of them but how would this compare to the cost of managing a separate repo and integrating the two mentioned above? The aexpect repo has not been very active in the last years but this could be viewed both as something good and something not so good (example of the second being contributors that did not want to use it because they thought the project has been abandoned at least in their replies to me).

So all in all I would be glad to hear your reasoning regarding keeping an already small project like aexpect bare bone and core-only since at least in my opinion it would rather make sense to split into multiple repos any projects that are much larger than this so that the costs from the splitting are outweighed by the benefits of separate maintenance.

[ldoktor]

Hi @ldoktor,

Hello @pevogam, I gave it another try after this long time and my feeling is still the same. Although it simplifies certain workflows I don't think it fits aexpect library. I can imagine it as another library that would generate command strings that would be then consumed by aexpect (or other library that allows shell-like interface).

You might be able to conclude that from the current selection of commands but there are commands that would require only sendline, some would require sending special characters like ^C or ^D, others would require varied timeouts and arguments to the session.cmd calls or different session.cmd_* calls. So the current one-string-preparation-for-session-command approach would reduce a few of the simplest commands but will not generalize over future ones.

I haven't seen such commands in this PR and therefore I mentioned that solution. For that kind of usage one would indeed had to depend directly on session usage.

My main problem is the size, the higher-level aim with security checks that are present in many places but in another they are missing (eg. stat(a, "/tmp/a", fmt="%; echo this could as well be formatting your hard drive; ").

For sure it might have some limitations that we all expect to be pointed out during review and patched up before merging. I think you could notice that this pull request combines lots of different code from different authors in our team and previously added in different times when they were actually needed. So some unification is definitely to ask but I don't think will be hard to accomplish and this is what reviews are for - to discover maintainers' minimum uniform interface requirements (and leave some divergence that will be unavoidable aside).

I didn't noticed different authors but i do see different levels of automatic transformation of input params (which probably better describes what I was trying to say before. Some functions transform the flags and trying to be safe, some are not). It's hard to decide but in case we can not guarantee safety I would prefer not trying to be too smart as it might give people false impression the utility handles security for them. It does not and the underlying session uses raw bash and should be only used by people with some level of knowledge of the commands they are trying to execute.

I can create a repo for you and make you an admin of that, if you are interested, but I don't like the idea of having this in the core aexpect (note ideally I'd extract and publish the remote.py separately as well, but it's already there...)

Why would the remote be considered separate from aexpect? The entire point of our previous arguments in the past was that it makes more sense in aexpect than in Avocado VT and I think it extends the client module quite nicely adding the file transfer and session login/prompts support. The same can now be said about the remote_door module which extends this further with remote function execution and remote object method calls. But letting all of this aside, do you think that a separate project would make things easier? One thing we seem to have learned from the past is that fragmenting projects into too many small ones adds a lot of deployment, versioning, and maintenance overhead resulting in hours of packaging, debugging and testing of version compatibility and so on.

Although I mentioned separate repo, it doesn't have to be that way. Avocado also hosts optional plugins inside the main repo and they can be brought by installing "avocado-framework-plugin-$NAME". Something like that would be nice for aexpect. My reason is that I'm using (and promoting) aexpect as stable and simple utility for automation. Some developers at RH already caught it up and are using it for rebooting machines over serial console, or other simple tasks. I'm also using it in other projects but the cleverer it gets the bigger and potentially more dangerous it gets.

I consider aexpect something like cat. It performs well and allows basic operations. One could use tail for similar tasks and even more, but it is more complex and potentially dangerous. And then you have visual studio, which also can perform file operation, allows all kinds of extra features, but is heavy-weight and aimed for different usecases. Now the question is, is aexpect growing to be closer to tail or to vs?

In my ideal world we'd have probably just 1 repo, but 3 target packages "aexpect", "aexpect-remote" and "aexpect-ops". Would this work for you?

Anyway @clebergnu, @lmr please take a look at this and if you decide it belongs here, feel free to give it a try.

Another point I want to add here is that we find lots of contributors on the Avocado and Avocado VT side reimplementing a lot of these functions on top of a session again and again which confirms the need to have single reusable source that everybody could instead simply spend their time extending and saving the rest of their costs. As these utilities depend only on aexpect and all they require is a session I don't think it makes sense to keep them in Avocado or Avocado VT repos whcih specialize in something else.

And I am honestly sad about that. I have pinged people from avocado-vt to move up the priority of your PR to finally move avocado-vt to aexpect-remote.

Regarding a separate aexpect repo, can you elaborate more on what are the benefits from keeping current aexpect to the minimal code base and functionality it currently has? I would assume reduced maintenance cost and contributor activity is one of them but how would this compare to the cost of managing a separate repo and integrating the two mentioned above? The aexpect repo has not been very active in the last years but this could be viewed both as something good and something not so good (example of the second being contributors that did not want to use it because they thought the project has been abandoned at least in their replies to me).

So all in all I would be glad to hear your reasoning regarding keeping an already small project like aexpect bare bone and core-only since at least in my opinion it would rather make sense to split into multiple repos any projects that are much larger than this so that the costs from the splitting are outweighed by the benefits of separate maintenance.

I mentioned it earlier, basically size and security. Core aexpect is very stable, proven to work in many applications and it's API is clearly "dangerous". By adding upper layers people might get false impression of it to be higher-level safe tool, which it is not or it would take a long time to ensure and keep testing the security of it's layer.

[clebergnu]

Hi @pevogam, thanks for taking the time to compile and submit those utilities.

I won't try to get into each of the points discussed so far, but I'll try to optimize the discussion by talking about the most important ones IMO.

First, about whether this module fits well enough in the aexpect repo. My take is that it does extend aexpect in ways that are useful to some people and use cases. IMO, as long as the code matches the project's quality standards, including passing review on its technical merits, and does not add maintenance burden, it deserves to go in. A project's scope shouldn't be so set in stone, but should respond to what its users need from it.

Second, the organization of aexpect's first level "aexpect" directory never seemed like a "core" module to me. In fact, this is the location from where all API users actually import code from. I can't find a difference between from aexpect import remote and from aexpect import ops.

Finally, a new topic related to maintenance: it would be great if tests could be added. I understand these are simple wrappers, but as shown in the issue found during review, users and maintainers would benefit from having some coverage there.

[ldoktor]

Hi @pevogam, thanks for taking the time to compile and submit those utilities.

I won't try to get into each of the points discussed so far, but I'll try to optimize the discussion by talking about the most important ones IMO.

First, about whether this module fits well enough in the aexpect repo. My take is that it does extend aexpect in ways that are useful to some people and use cases. IMO, as long as the code matches the project's quality standards, including passing review on its technical merits, and does not add maintenance burden, it deserves to go in. A project's scope shouldn't be so set in stone, but should respond to what its users need from it.

Second, the organization of aexpect's first level "aexpect" directory never seemed like a "core" module to me. In fact, this is the location from where all API users actually import code from. I can't find a difference between from aexpect import remote and from aexpect import ops.

OK, I understand the point that we already have another "uglier" modules in there. I hope I'll find some time to come-up with a simple to use way to separate them (ideally multiple pip packages generated out of a single source-tree) but until then let's get this in. I mean it is useful, it is way cleaner than remote and Plamen (and others) spent already way too much on updating it.

Finally, a new topic related to maintenance: it would be great if tests could be added. I understand these are simple wrappers, but as shown in the issue found during review, users and maintainers would benefit from having some coverage there.

Yep, selftests are always welcome, although even the core aexpect contains only a bare minimum selftests.

[pevogam]

Hi @clebergnu and @ldoktor, thank you both for spending some of your valuable time on this. I have updated the pull request with all of your comments. I realize and fully agree the code is not of the best quality and I apologize if I am wasting your precious brain cycles on this.

My approach and proposal on how to proceed here is rather simple: we try to satisfy your requirements for some minimum quality in order for the code to become available for others and then we can have any or lots of further improvements to make it more proper and satisfactory depending on demand, i.e. we can always redirect devs that would otherwise reinvent the wheel to solve their own needs as contributors here. In this way it can also be of less effort for you and we can have the highest rates of availability and opportunity for further development with the minimum needed amount of effort.

On my side I will make sure to add some follow up contributions regarding this module as potential subpackage that would address larger refactoring requests and am always available to help with maintenance if you so desire.

[pevogam]

A quick comment regarding the unit tests: I assume this would imply some mocking and basic command checks (even though I am not sure how much it makes sense since it is not as good as functional tests) but I can do so in the "larger refactoring" part mentioned above.

[ldoktor]

As for me the selftests would be nice but not mandatory at this point. Mocking could consist of simply passing a mock object and checking the output values (ensuring quotation is enforced, arguments are transformed, expected calls are being performed and data are being processed correctly)

[pevogam]

As for me the selftests would be nice but not mandatory at this point. Mocking could consist of simply passing a mock object and checking the output values (ensuring quotation is enforced, arguments are transformed, expected calls are being performed and data are being processed correctly)

This is what I would expect via mocking too but

1. I have had similar tests for a similarly "shallow" code before and the test code is consists almost of a single line that tests the one-line implementation and thus almost black-box-reimplements it - something that I am not particularly fond of. There is the coverage based on actually "running" the code that is needed for interpreted languages like python here but that is then much better covered by a functional test where any changing API of or resulting incompatibility with the underlying binaries will be easily detected (one of the main points of usefulness here) and anything else would require one to update the mock object's validation all the time in order to be up to date with the changing binaries. Granted, this is not exactly the case here consider the age of the cli interface of these binaries but I guess my main point is that the unit tests have to be more carefully considered in order to actually be useful and not fragile, having to be updated every few commits.

2. The signature and output of functions is still not standardized well enough in order to validate sufficient consistency with a pass grade. This is better looked at after contributing more functions and reaching a more final shape of the session ops module.

[pevogam]

I tried pushing on most recent master branch to trigger the CI again (didn't trigger within 6 hour timeout upon previous push) but it seems it still won't start. I will however run our functional tests or a good portion of them with the current changes just in case.

[lmr]

Hi @pevogam, thank you so much for the library.

I read the discussion so far, and here are my 2 cents about your code:

1. I generally like the idea of having these shell functions (or operations) as a library, as they are widely used in a variety of situations. Having them neatly packaged is a good idea.
   As @clebergnu mentioned, as long as we meet the necessary criteria for inclusion (as far as I can see the code generally is clean and documented), it makes for a good inclusion.

2. Creating an extra separate library to interact with aexpect might be too much granularity, as this is fairly small and will be mostly concentrated in the client library. We could think of an aexpect-extensions library if we ever get too many extra functions, but that doesn't seem very likely at the moment.

3. As far as code organization style goes, all these functions take session as the first parameter. Have you considered making the functions an object that inherits from session? That's something like:

from aexpect.remote.ops import ShellOpsSession

session = ShellOpsSession("sh")
session.stat(path="/tmp/a")
session.copy(src="/tmp/a", dst="/tmp/b")

That saves a bit of typing and it seems like a reasonable layering of functionality.

Of course if @clebergnu and @ldoktor disagree, please let me know if I'm off here.

Once again, thanks for your contribution!

[pevogam]

Hi @lmr,

1. I generally like the idea of having these shell functions (or operations) as a library, as they are widely used in a variety of situations. Having them neatly packaged is a good idea.
   As @clebergnu mentioned, as long as we meet the necessary criteria for inclusion (as far as I can see the code generally is clean and documented), it makes for a good inclusion.

Thanks both for chiming in with views on this, I agree that the current functionality is quite skinny when it comes to wrapping logic around the shell command strings but it could easily grow into some extended and extra useful logic too.

2. Creating an extra separate library to interact with aexpect might be too much granularity, as this is fairly small and will be mostly concentrated in the client library. We could think of an `aexpect-extensions` library if we ever get too many extra functions, but that doesn't seem very likely at the moment.

Yes, I am all in for separating if this becomes big enough later on but just like you I think that at the moment creating separate project infrastructure is a bit of an overkill.

3. As far as code organization style goes, all these functions take session as the first parameter. Have you considered making the functions an object that inherits from `session`? That's something like:

from aexpect.remote.ops import ShellOpsSession

session = ShellOpsSession("sh")
session.stat(path="/tmp/a")
session.copy(src="/tmp/a", dst="/tmp/b")

That saves a bit of typing and it seems like a reasonable layering of functionality.

I can see three problems with this:

1. Typing session.copy(A, B) is not much shorter than typing ops.copy(session, A, B) (about 3+ chars difference).
2. More importantly, adding more functions later on would require us to move from ops.py module to ops subpackage with the current sections becoming separate modules as was recommended by some of our previous reviewers. Extending a class over multiple module seems somewhat incorrect and it would make more sense for people to just import an ops subpackage or modules and functions thereof. Note that this is still an intermediate stage that would take a while before coming to the need to talk about a separate project of course.
3. There are other utilities like remote, remote_door, and so on that already operate with similar function signature with a session first argument.

[ldoktor]

I'm still not happy about this not being a separate module, but it looks good and others are fine having it in-tree, so I think it's ready for inclusion. Maybe one thing to consider is the strip in the cat method. I do understand why it's there, but maybe it'd be safer to leave it on the user. Anyway let's not block this anymore and include this first implementation, we can tweak it later, if necessary.

[ldoktor]

Well it re-runs with the same base commit, anyway passes local "make check" so let me merge this.

[pevogam]

Hi @ldoktor and thanks all for all the reviews here,

Well it re-runs with the same base commit, anyway passes local "make check" so let me merge this.

Hmm but as I wrote above the situation with the ls and status 2 is still not resolved. Should I write an issue about it or propose a pull request? Could you still comment on #98 (comment) above?