[common-utils] - append shell history functionality - #1026

[gauravsaini04]

Ref: #1026

Feature

• Common-Utils

Description

• In response to Common-utils feature should include shell history #1026 have appended shell history functionality to common-utils feature as a configurable option

changelog

• Have added changes to devcontainer-feature.json file, added boolean option allowShellHistory and mounts as a configurable option
• Have added changes to main.sh in common-utils features src
• Have added to scenarios to test this new feature
• Have added test cases in allow_shell_history.sh file

Checklist

• checked that applied changes work as expected

[eitsupi]

Some questions:

• Wouldn't adding more functionality to common-utils here go against this discussion? #67
• Why not use lifecycle-scripts? See stuartleeks/dev-container-features#16

[eitsupi]

Why add this functionality to common-utils instead of relying on another Feature?
Wouldn't common-utils be too heavy a dependency for users who only want to use history?

And I am also convinced that the lifecycle script approach is far superior when implementing such a functionality.
Please check stuartleeks/dev-container-features#16

[gauravsaini04]

And I am also convinced that the lifecycle script approach is far superior when implementing such a functionality.

Thanks for valuable inputs @eitsupi, waiting for the logic to be approved by @chrmarti, will be converting to a lifecycle script after this

[chrmarti]

Why add this functionality to common-utils instead of relying on another Feature? Wouldn't common-utils be too heavy a dependency for users who only want to use history?

I agree, I'm also not sure if adding a devcontainers volume for all users of our base images will be ideal. Though having persistence for shell history built-in would be a great feature.

And I am also convinced that the lifecycle script approach is far superior when implementing such a functionality. Please check stuartleeks/dev-container-features#16

This seems to require password-less sudo access. The feature would not work without that.

[eitsupi]

This seems to require password-less sudo access. The feature would not work without that.

Isn't that irrelevant whether or not lifecycle scripts are used? We don't know the permissions of a mounted volume until it is mounted, do we?

[Kaniska244]

Hi @chrmarti ,

Further changes done to implement a new test case for bash history retention logic. Would you kindly review the changes & provide comments on the same.

With Regards,
Kaniska Sengupta

[chrmarti]

One issue with the change is that it unconditionally adds mount points independent of whether preserving the shell history is enabled or not. Given that the common-utils are in all our base images, this would affect a lot of dev containers. Possible approaches to avoid this are:

• Add support for conditional mount points for features in the spec and CLI.
• Move the functionality to a separate feature. Users will have to add it explicitly.
• Make support for preserving the shell history part of the CLI. (This could allow us to reuse the vscode volume because the Dev Containers extension could tell the CLI to use that. It might also allow us to enable it by default with a setting in the Dev Containers extension.)

I suggest we put this on hold for now.