Skip to content

fix(shell-history): fix broken fish config script #22

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.

Sign up for GitHub

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

Merged
stuartleeks merged 11 commits into from
Mar 24, 2025
Merged

fix(shell-history): fix broken fish config script #22

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
f41df53
fix(shell-history): fix fish sytax and ensure creating the sym link
eitsupi Mar 19, 2025
d6f4cf1
ci: test fish config
eitsupi Mar 19, 2025
0b2a4be
ci: ensure run tests on PRs
eitsupi Mar 19, 2025
1900ea7
test(shell-history): use the latest fish feature
eitsupi Mar 19, 2025
4d98d3a
ci: use test.yaml from the latest feature-starter template
eitsupi Mar 19, 2025
32564ac
test: fix test for fish
eitsupi Mar 19, 2025
e7c7a84
test: fix test name
eitsupi Mar 19, 2025
7ef02f3
chore(shell-history): bump version
eitsupi Mar 20, 2025
d94314d
refactor(shell-history): use single quote
eitsupi Mar 20, 2025
b5cea4b
test(shell-history): more tests for fish
eitsupi Mar 20, 2025
3016992
ci: remove failing test for now
eitsupi Mar 24, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 29 additions & 13 deletions .github/workflows/test.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,36 +3,52 @@ on:
push:
branches:
- main
pull_request:
workflow_dispatch:

jobs:
test:
test-autogenerated:
runs-on: ubuntu-latest
# continue-on-error: true
continue-on-error: true
strategy:
matrix:
include:
- features: "dev-tunnel"
baseImage: "mcr.microsoft.com/vscode/devcontainers/javascript-node:0-18"
remoteUser: node
- features: "dev-tunnel"
baseImage: "mcr.microsoft.com/devcontainers/base:ubuntu"
remoteUser: root
features:
- shell-history
baseImage:
- debian:latest
- ubuntu:latest
- mcr.microsoft.com/devcontainers/base:ubuntu
steps:
- uses: actions/checkout@v3
- uses: actions/checkout@v4

- name: "Install latest devcontainer CLI"
run: npm install -g @devcontainers/cli

- name: "Generating tests for '${{ matrix.feature }}' against '${{ matrix.baseImage }}'"
run: devcontainer features test --features ${{ matrix.feature }} --base-image ${{ matrix.baseImage }} --remote-user ${{ matrix.remoteUser }} .
- name: "Generating tests for '${{ matrix.features }}' against '${{ matrix.baseImage }}'"
run: devcontainer features test --skip-scenarios -f ${{ matrix.features }} -i ${{ matrix.baseImage }} .

test-scenarios:
runs-on: ubuntu-latest
continue-on-error: true
strategy:
matrix:
features:
- dev-tunnels
- shell-history
steps:
- uses: actions/checkout@v4

- name: "Install latest devcontainer CLI"
run: npm install -g @devcontainers/cli

- name: "Generating tests for '${{ matrix.features }}' scenarios"
run: devcontainer features test -f ${{ matrix.features }} --skip-autogenerated --skip-duplicated .

# test-global:
# runs-on: ubuntu-latest
# continue-on-error: true
# steps:
# - uses: actions/checkout@v2
# - uses: actions/checkout@v4

# - name: "Install latest devcontainer CLI"
# run: npm install -g @devcontainers/cli
Expand Down
4 changes: 2 additions & 2 deletions src/shell-history/devcontainer-feature.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
{
"name": "Shell History",
"id": "shell-history",
"version": "0.0.5",
"version": "0.0.6",
"description": "Preserve shell history across dev container instances. Currently supports bash, zsh, and fish",
"options": {},
"mounts": [
Expand All @@ -18,4 +18,4 @@
"onCreateCommand": {
"shell-history": "/usr/local/share/stuartleeks-devcontainer-features/shell-history/scripts/oncreate.sh"
}
}
}
17 changes: 8 additions & 9 deletions src/shell-history/oncreate.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,19 +42,18 @@ EOF

# Create symlink for fish
mkdir -p $HOME/.config/fish
cat << EOF >> "$HOME/.config/fish/config.fish"
if [ -z "\$XDG_DATA_HOME" ];
then
cat << 'EOF' >> "$HOME/.config/fish/config.fish"
if [ -z "$XDG_DATA_HOME" ];
set history_location ~/.local/share/fish/fish_history
else
set history_location \$XDG_DATA_HOME/fish/fish_history
fi
Copy link
Owner

@stuartleeks stuartleeks Mar 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this work for you without the escaping?

I had to add the escaping back in for the bash history as otherwise the $XDG_DATA_HOME env var is evaluated at the point that oncreate.sh runs and that wasn't set Escaping it ensured that it was evaluated when the .bashrc was run.

Copy link
Contributor Author

@eitsupi eitsupi Mar 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your review.

IIUC, replacing EOF to "EOF" allows us to use $ without escaping here.
https://en.wikipedia.org/wiki/Here_document

But from this Wikipedia page, perhaps single quoted 'EOF' might be better?

Copy link
Owner

@stuartleeks stuartleeks Mar 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah - I'll file that under Things I Learned 😄

In terms of single vs double quoting, I think I prefer single as the article suggests

set history_location $XDG_DATA_HOME/fish/fish_history
end

if [ -f \$history_location ]; then
mv \$history_location "\$history_location-old"
fi
if [ -f $history_location ];
mv $history_location "$history_location-old"
end

ln -s /dc/shellhistory/fish_history \$history_location
ln -fs /dc/shellhistory/fish_history $history_location
EOF

fix_permissions /dc/shellhistory
8 changes: 7 additions & 1 deletion test/shell-history/fish_shell.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,4 +2,10 @@

set -e

./_default.sh
# Optional: Import test library bundled with the devcontainer CLI
source dev-container-features-test-lib

# Feature-specific tests
check "check fish config script validity" fish -c "source $HOME/.config/fish/config.fish"
check "check fish config script content" bash -c "cat $HOME/.config/fish/config.fish | grep -q 'XDG_DATA_HOME'"
Copy link
Contributor Author

@eitsupi eitsupi Mar 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have added a test. I believe this test confirms that $XDG_DATA_HOME is present in the script.

check "cache dir permission" bash -c "test -w /dc/shellhistory"
4 changes: 2 additions & 2 deletions test/shell-history/scenarios.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
"fish_shell": {
"image": "mcr.microsoft.com/devcontainers/base:debian",
"features": {
"ghcr.io/meaningful-ooo/devcontainer-features/fish:1": {},
"ghcr.io/meaningful-ooo/devcontainer-features/fish:latest": {},
"shell-history": {}
}
},
Expand All @@ -28,4 +28,4 @@
},
"remoteUser": "root"
}
}
}