-
Notifications
You must be signed in to change notification settings - Fork 416
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
Common-utils: various cleaning things from other pr. #653
Changes from 8 commits
f5b5e6a
6e635a6
f279594
b40bbd6
431729f
108ab53
4c68aee
dc73b2d
3150f0d
836f9e9
1724e14
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -427,7 +427,27 @@ if [ "${RC_SNIPPET_ALREADY_ADDED}" != "true" ]; then | |
fi | ||
|
||
# Optionally configure zsh and Oh My Zsh! | ||
omz_rc_filename=".zshrc" | ||
omz_source_dirname=".oh-my-zsh" | ||
user_rc_file="${user_home}/${omz_rc_filename}" | ||
user_omz_install_dir="${user_home}/${omz_source_dirname}" | ||
template_path="${user_omz_install_dir}/templates/zshrc.zsh-template" | ||
|
||
# Allow upstream steps to use installOhMyZshConfig false | ||
# Given previous step configured ~/.zshrc, | ||
# When installOhMyZshConfig is false, done before INSTALL_ZSH since | ||
# Or where installOhMyZshconfig false, and installZsh false | ||
# Then remove the file | ||
if [ "$OH_MY_ZSH_CONFIG_INSTALLED" = "true" ] && [ "$INSTALL_OH_MY_ZSH_CONFIG" = "false" ]; then | ||
if [ -f "${user_rc_file}" ]; then | ||
rm "${user_rc_file}" | ||
OH_MY_ZSH_CONFIG_INSTALLED="false" | ||
fi | ||
fi | ||
|
||
if [ "${INSTALL_ZSH}" = "true" ]; then | ||
umask g-w,o-w | ||
|
||
if [ "${ZSH_ALREADY_INSTALLED}" != "true" ]; then | ||
if [ "${ADJUSTED_ID}" = "rhel" ]; then | ||
global_rc_path="/etc/zshrc" | ||
|
@@ -450,46 +470,52 @@ if [ "${INSTALL_ZSH}" = "true" ]; then | |
chsh --shell /bin/zsh ${USERNAME} | ||
fi | ||
|
||
# Adapted, simplified inline Oh My Zsh! install steps that adds, defaults to a codespaces theme. | ||
# See https://github.com/ohmyzsh/ohmyzsh/blob/master/tools/install.sh for official script. | ||
if [ "${INSTALL_OH_MY_ZSH}" = "true" ]; then | ||
user_rc_file="${user_home}/.zshrc" | ||
oh_my_install_dir="${user_home}/.oh-my-zsh" | ||
template_path="${oh_my_install_dir}/templates/zshrc.zsh-template" | ||
if [ ! -d "${oh_my_install_dir}" ]; then | ||
umask g-w,o-w | ||
mkdir -p ${oh_my_install_dir} | ||
# Adapted, simplified inline Oh My Zsh! install steps that adds, defaults to a codespaces theme. | ||
# See https://github.com/ohmyzsh/ohmyzsh/blob/master/tools/install.sh for official script. | ||
omz_added_filesnames=("${omz_source_dirname}") | ||
if [ ! -d "${user_omz_install_dir}" ]; then | ||
mkdir -p ${user_omz_install_dir} | ||
git clone --depth=1 \ | ||
-c core.eol=lf \ | ||
-c core.autocrlf=false \ | ||
-c fsck.zeroPaddedFilemode=ignore \ | ||
-c fetch.fsck.zeroPaddedFilemode=ignore \ | ||
-c receive.fsck.zeroPaddedFilemode=ignore \ | ||
"https://github.com/ohmyzsh/ohmyzsh" "${oh_my_install_dir}" 2>&1 | ||
|
||
"https://github.com/ohmyzsh/ohmyzsh" "${user_omz_install_dir}" 2>&1 | ||
# Shrink git while still enabling updates | ||
cd "${oh_my_install_dir}" | ||
git repack -a -d -f --depth=1 --window=1 | ||
GIT_WORK_TREE="${user_omz_install_dir}" GIT_DIR="${user_omz_install_dir}/.git" git repack\ | ||
-a -d -f --depth=1 --window=1 | ||
fi | ||
|
||
# Add Dev Containers theme | ||
mkdir -p ${oh_my_install_dir}/custom/themes | ||
cp -f "${FEATURE_DIR}/scripts/devcontainers.zsh-theme" "${oh_my_install_dir}/custom/themes/devcontainers.zsh-theme" | ||
ln -sf "${oh_my_install_dir}/custom/themes/devcontainers.zsh-theme" "${oh_my_install_dir}/custom/themes/codespaces.zsh-theme" | ||
# Add dev containers theme | ||
user_omz_theme_filepath="${user_omz_install_dir}/custom/themes" | ||
user_devcontainer_theme_target="${user_omz_theme_filepath}/devcontainers.zsh-theme" | ||
user_codespaces_theme_target="${user_omz_theme_filepath}/codespaces.zsh-theme" | ||
theme_template_path="${FEATURE_DIR}/scripts/devcontainers.zsh-theme" | ||
mkdir -p "${user_omz_theme_filepath}" | ||
cp -f "${theme_template_path}" "${user_devcontainer_theme_target}" | ||
cp -f "${theme_template_path}" "${user_codespaces_theme_target}" | ||
|
||
# Add devcontainer .zshrc template | ||
if [ "$INSTALL_OH_MY_ZSH_CONFIG" = "true" ]; then | ||
echo -e "$(cat "${template_path}")\nDISABLE_AUTO_UPDATE=true\nDISABLE_UPDATE_PROMPT=true" > ${user_rc_file} | ||
sed -i -e 's/ZSH_THEME=.*/ZSH_THEME="devcontainers"/g' ${user_rc_file} | ||
OH_MY_ZSH_CONFIG_INSTALLED="true" | ||
omz_added_filesnames+=("${omz_rc_filename}") | ||
fi | ||
|
||
# Copy to non-root user if one is specified | ||
user_omz_filepaths=( "${omz_added_filesnames[@]/#/$user_home/}" ) | ||
|
||
if [ "${USERNAME}" != "root" ]; then | ||
copy_to_user_files=("${oh_my_install_dir}") | ||
[ -f "$user_rc_file" ] && copy_to_user_files+=("$user_rc_file") | ||
cp -rf "${copy_to_user_files[@]}" /root | ||
chown -R ${USERNAME}:${group_name} "${oh_my_install_dir}" "${user_rc_file}" | ||
# Copy files to alternate user if one is specified | ||
cp -rf "${user_omz_filepaths[@]}" /root | ||
# Set permissions for root user | ||
chown -R root:root "${omz_added_filesnames[@]/#//root/}" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Curious, why is this needed? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Security. User scripts making it into root and being sourced. Would be tempted to copy from a mktmp-d. To prevent copying any unknown files Ie custom shell scripts Nvm. I will close pr. |
||
fi | ||
|
||
# Set permissions for current user | ||
chown -R "${USERNAME}:${group_name}" "${user_omz_filepaths[@]}" | ||
fi | ||
fi | ||
|
||
|
@@ -531,6 +557,7 @@ echo -e "\ | |
LOCALE_ALREADY_SET=${LOCALE_ALREADY_SET}\n\ | ||
EXISTING_NON_ROOT_USER=${EXISTING_NON_ROOT_USER}\n\ | ||
RC_SNIPPET_ALREADY_ADDED=${RC_SNIPPET_ALREADY_ADDED}\n\ | ||
ZSH_ALREADY_INSTALLED=${ZSH_ALREADY_INSTALLED}" > "${MARKER_FILE}" | ||
ZSH_ALREADY_INSTALLED=${ZSH_ALREADY_INSTALLED}\n\ | ||
OH_MY_ZSH_CONFIG_INSTALLED=${OH_MY_ZSH_CONFIG_INSTALLED}" > "${MARKER_FILE}" | ||
|
||
echo "Done!" |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
#!/bin/bash | ||
|
||
set -e | ||
|
||
# Optional: Import test library | ||
source dev-container-features-test-lib | ||
|
||
# Definition specific tests | ||
check "default-zsh-with-no-zshrc" bash -c "[ ! -e ~/.zshrc ]" | ||
|
||
# Report result | ||
reportResults |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,6 +13,8 @@ check "zsh" zsh --version | |
check "ps" ps --version | ||
check "Oh My Zsh! theme" test -e $HOME/.oh-my-zsh/custom/themes/devcontainers.zsh-theme | ||
check "zsh theme symlink" test -e $HOME/.oh-my-zsh/custom/themes/codespaces.zsh-theme | ||
check "zsh theme filename" test -e $HOME/.oh-my-zsh/custom/themes/codespaces.zsh-theme | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would take the meaning of -e and symlink to mean xor... not sure @samruddhikhandale |
||
check "oh-my-zsh executes" zsh -c 'source $HOME/.zshrc && echo $0 | grep zsh' | ||
|
||
# Report result | ||
reportResults |
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.
With the PR changes
installOhMyZshConfig
would behave completely different as compared to other Feature options.For eg. Let's say an image (say
X
) installs common-utils as follows -Then a user creates their
devcontainer.json
as 👇Does this mean the Feature should remove the
zsh
installation? I don't think that the Features should remove an existing functionality, however, should only update or add new tools/configs/themes.When a user add a Feature to their
devcontainer.json
, it is assumed that the"installZsh": false
option would NOT installzsh
but it doesn't mean that it should remove the already existingzsh
. This behavior could create confusions and removing something is always a bit risky. I think we should avoid this approach.@naturedamends Let me know what you think!
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.
Maybe, remove the file, if zshrc exists and installZshConfig false and installZsh true?
Or do you not like the file being removed at all?
Should I remove the source of zshrc added in tests? Such that it's not executed within within build runtime. Then again.. it's a container
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.
This ☝️
You never know if the zsh config has been modified by the dockerfiles (or something else), hence, it's best to avoid removing anything. Also, removing config makes sense only if the Feature had an option like
removeZshConfig
, but doesn't seem like that's needed.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.
Hose about linking ~/.zshrc to .zshrc.devcontainer.
Then downstream can handle, or ln -sf?