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

gauravsaini04 · 2024-10-22T08:27:40Z

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? Consider splitting up the common feature #67
Why not use lifecycle-scripts? See Use Lifecycle Scripts stuartleeks/dev-container-features#16

…_common-utils_integration

chrmarti · 2024-10-29T08:24:17Z

src/common-utils/devcontainer-feature.json

+    },
+    "mounts": [
+        {
+            "source": "${devcontainerId}-shellhistory",


This will add a volume for each container which will make volumes harder to manage for the user. Maybe you could use a single volume devcontainers at /devcontainers and in that create a folder /devcontainers/shellHistory/${devcontainerId} for each dev container.

have added logic such that uuidgen utility is used for generating random devcontainer id for a container, intent is to embed it inside the folder structure like this :
/devcontainers/$(uuidgen)/shellHistory for a symlink to the ~/.bash_history being kept here. Logic is working for retention of shell history, keeping it as draft so you can check for bash shell

chrmarti · 2024-10-29T08:29:17Z

src/common-utils/main.sh

+if [[ -z "\$HISTFILE_OLD" ]]; then
+    export HISTFILE_OLD=\$HISTFILE
+fi
+export HISTFILE=/dc/shellhistory/.bash_history


Would there be value in keeping the default path ~/.bash_history and instead make that a symbolic link to the actual file in the volume? (I haven't tried using a symbolic link, best to check first if bash/zsh work correctly with that.)

done like this. thanks.

…y bash right now

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 · 2024-11-03T00:39:08Z

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 · 2024-11-04T09:51:17Z

src/common-utils/main.sh

+        sudo apt-get install -y uuid-runtime
+    fi
+    # Create the shell history directory in the mounted volume
+    DEVCONTAINER_ID=$(uuidgen)


One downside of generating a UUID here is that it will change with each rebuild of the container image. The ${devcontainerId} value could be passed into the container with "containerEnv" (it could make sense for the CLI to do this automatically, but it currently doesn't) where this script could use it when run as part of the "postCreateCommand". 🤔 (It would also make sense for the CLI to pass DEVCONTAINER_ID into the feature install scripts.)

made changes as suggested.

chrmarti · 2024-11-04T09:59:35Z

src/common-utils/main.sh

+    # Ensure the volume's history file exists and set permissions
+    if [[ ! -f "${VOLUME_HISTORY_FILE}" ]]; then
+        # Create an empty history file if it doesn’t already exist
+        sudo touch "${VOLUME_HISTORY_FILE}"


I think this won't work when the volume already exists because the volume is not mounted at this point and the image contents at /devcontainer will be copied to the volume only once - when the volume is created.

removed the check

chrmarti · 2024-11-04T10:00:37Z

src/common-utils/main.sh

+
+    # Create or update the user’s .bash_history to append to the volume’s history
+    if [[ ! -f "${USER_HISTORY_FILE}" ]]; then
+        sudo touch "${USER_HISTORY_FILE}"


This will be overwritten by the symbolic link below. I think this can be removed.

chrmarti · 2024-11-04T10:09:53Z

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 · 2024-11-06T13:18:23Z

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?

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

7cbb7de

eitsupi reviewed Oct 22, 2024

View reviewed changes

Merge branch 'devcontainers:main' into features_#1026_shellhistory_in…

412d221

…_common-utils_integration

gauravsaini04 marked this pull request as ready for review October 28, 2024 22:26

gauravsaini04 requested a review from a team as a code owner October 28, 2024 22:26

chrmarti requested changes Oct 29, 2024

View reviewed changes

gauravsaini04 added 2 commits November 3, 2024 00:22

[common-utils] - added working shell history preserving logic for onl…

3076345

…y bash right now

change to mounts formatting

76d048b

eitsupi suggested changes Nov 3, 2024

View reviewed changes

gauravsaini04 marked this pull request as draft November 3, 2024 00:31

chrmarti requested changes Nov 4, 2024

View reviewed changes

gauravsaini04 added 17 commits November 5, 2024 01:27

changes as suggested

192e4f0

changed path of source for bind mount

930050d

changes to path yet again for source of bind mount

6970923

change

5c7b513

point to correct source dir

753ab15

small change to path of source file

d21cc08

change

e48c1c0

check root folder

10747ff

change

581e5eb

misc

54119e3

path correction final

dd34103

change

ba649cf

try

c1b37fa

c

c227dc8

s

1de5f38

hi

50ff6d2

fin

129f6bf

gauravsaini04 added 2 commits November 5, 2024 06:11

c

c7c5aa8

Update devcontainer-feature.json

3f1e7c9

gauravsaini04 added 3 commits November 8, 2024 03:58

giving due permissions before mounts is called for file move

be92e63

few changes

dec0296

change

261428f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

gauravsaini04 commented Oct 22, 2024

eitsupi left a comment

chrmarti Oct 29, 2024

gauravsaini04 Nov 3, 2024 •

edited

Loading

chrmarti Oct 29, 2024

gauravsaini04 Nov 3, 2024

eitsupi left a comment

gauravsaini04 commented Nov 3, 2024

chrmarti Nov 4, 2024

gauravsaini04 Nov 5, 2024

chrmarti Nov 4, 2024

gauravsaini04 Nov 5, 2024

chrmarti Nov 4, 2024

gauravsaini04 Nov 5, 2024

chrmarti commented Nov 4, 2024

eitsupi commented Nov 6, 2024

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

Are you sure you want to change the base?

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

Conversation

gauravsaini04 commented Oct 22, 2024

eitsupi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gauravsaini04 Nov 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eitsupi left a comment

Choose a reason for hiding this comment

gauravsaini04 commented Nov 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrmarti commented Nov 4, 2024

eitsupi commented Nov 6, 2024

gauravsaini04 Nov 3, 2024 •

edited

Loading