Common-utils: various cleaning things from other pr. by naturedamends · Pull Request #653 · devcontainers/features

naturedamends · 2023-08-13T20:32:02Z

Proposal

Enable usage of the installOhMyZshConfig flag within Microsoft base dev container images.

Since images may have had previous installs which come with commom-utils. This allows the config option to function as stated.

failed proposal

This is now just cleaning up the file and fixes some issues with the flag for non root usernames.

naturedamends · 2023-08-13T20:33:29Z

Tests are failing locally due to a permissions issue writing to ~/.oh-my-zsh owned by vscode as user root.

Present in debian:bullseye and alpine

#14 34.71 USERNAME: vscode
ls -la /root
#14 34.71 total 20
#14 34.71 drwx------ 1 root root 4096 Jul 25 00:00 .
#14 34.71 drwxr-xr-x 1 root root 4096 Aug 13 19:51 ..
#14 34.71 -rw-r--r-- 1 root root 2026 Aug 13 19:52 .bashrc
#14 34.71 -rw-r--r-- 1 root root  161 Jul  9  2019 .profile
ls -la /home/vscode
#14 34.71 total 24
#14 34.71 drwxr-xr-x 2 vscode vscode 4096 Aug 13 19:52 .
#14 34.71 drwxr-xr-x 1 root   root   4096 Aug 13 19:52 ..
#14 34.71 -rw-r--r-- 1 vscode vscode  220 Mar 27  2022 .bash_logout
[2023-08-13T19:52:32.078Z] 
#14 34.71 -rw-r--r-- 1 vscode vscode 4981 Aug 13 19:52 .bashrc
#14 34.71 -rw-r--r-- 1 vscode vscode  807 Mar 27  2022 .profile
#14 34.71 Cloning into '/home/vscode/.oh-my-zsh'...

[2023-08-13T19:52:32.976Z] #14 35.72 chown: cannot access '/home/vscode/.zshrc': No such file or directory

Samruddhi Khandale (samruddhikhandale)

Thanks for contributing the PR, left some comments!

naturedamends · 2023-08-15T13:01:22Z

Testing the marked flag I have is a little hard since I would need to build a base image within a test, since the avaliable base images have old version of the source code, and thus no flag.

Samruddhi Khandale (samruddhikhandale) · 2023-08-15T21:32:38Z

Looks like the recent commits are causing multiple test failures. See https://github.com/devcontainers/features/actions/runs/5868233855?pr=653

naturedamends · 2023-08-16T20:02:02Z

Do you understand this -- error with the coreutils cp? Samruddhi Khandale (@samruddhikhandale) . Couldn't re create locally

cp -f /tmp/dev-container-features/common-utils_0/scripts/devcontainers.zsh-theme /home/vscode/.oh-my-zsh/custom/themes/devcontainers.zsh-theme
#14 0.396 + cp -f /home/vscode/.oh-my-zsh/custom/themes/devcontainers.zsh-theme /home/vscode/.oh-my-zsh/custom/themes/codespaces.zsh-theme
#14 0.397 cp: '/home/vscode/.oh-my-zsh/custom/themes/devcontainers.zsh-theme' and '/home/vscode/.oh-my-zsh/custom/themes/codespaces.zsh-theme' are the same file

from this code

        # 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"
        cp -f "${oh_my_install_dir}/custom/themes/devcontainers.zsh-theme" "${oh_my_install_dir}/custom/themes/codespaces.zsh-theme"

see extended log
6_test (common-utils, mcr.microsoft.comdevcontainersbaseubun.txt

See local test (issue not reproduced):

╭─dev at iMac in /opt/workspace/gists/cp-testing 23-08-16 - 22:22:53
╰─○ cat script
#!/bin/bash


cd "/home/vscode"
pwd

mkdir source
echo "testing" >> source/source
mkdir target

cp -vf source/source target/codespace
cp -vf target/codespace target/devcontainers

stat target/*
╭─dev at iMac in /opt/workspace/gists/cp-testing 23-08-16 - 22:23:11
╰─○ docker run -it -v $(pwd):/app -w /app --rm  mcr.microsoft.com/devcontainers/base:ubuntu  bash /app/script
/home/vscode
'source/source' -> 'target/codespace'
'target/codespace' -> 'target/devcontainers'
  File: target/codespace
  Size: 8               Blocks: 8          IO Block: 4096   regular file
Device: 89h/137d        Inode: 6571041     Links: 1
Access: (0644/-rw-r--r--)  Uid: (    0/    root)   Gid: (    0/    root)
Access: 2023-08-16 21:23:14.618101122 +0000
Modify: 2023-08-16 21:23:14.613101122 +0000
Change: 2023-08-16 21:23:14.613101122 +0000
 Birth: 2023-08-16 21:23:14.612101122 +0000
  File: target/devcontainers
  Size: 8               Blocks: 8          IO Block: 4096   regular file
Device: 89h/137d        Inode: 6571042     Links: 1
Access: (0644/-rw-r--r--)  Uid: (    0/    root)   Gid: (    0/    root)
Access: 2023-08-16 21:23:14.618101122 +0000
Modify: 2023-08-16 21:23:14.618101122 +0000
Change: 2023-08-16 21:23:14.618101122 +0000
 Birth: 2023-08-16 21:23:14.618101122 +0000

test with absolute paths

docker run -it -v $(pwd):/app -w /app --rm  mcr.microsoft.com/devcontainers/base:ubuntu  bash /app/script
/home/vscode
'/home/vscode/source/source' -> '/home/vscode/target/codespace'
'/home/vscode/target/codespace' -> '/home/vscode/target/devcontainers'
  File: target/codespace
  Size: 8               Blocks: 8          IO Block: 4096   regular file
Device: 89h/137d        Inode: 6571048     Links: 1
Access: (0644/-rw-r--r--)  Uid: (    0/    root)   Gid: (    0/    root)
Access: 2023-08-16 21:26:47.338729331 +0000
Modify: 2023-08-16 21:26:47.334729331 +0000
Change: 2023-08-16 21:26:47.334729331 +0000
 Birth: 2023-08-16 21:26:47.334729331 +0000
  File: target/devcontainers
  Size: 8               Blocks: 8          IO Block: 4096   regular file
Device: 89h/137d        Inode: 6571049     Links: 1
Access: (0644/-rw-r--r--)  Uid: (    0/    root)   Gid: (    0/    root)
Access: 2023-08-16 21:26:47.338729331 +0000
Modify: 2023-08-16 21:26:47.338729331 +0000
Change: 2023-08-16 21:26:47.338729331 +0000
 Birth: 2023-08-16 21:26:47.338729331 +0000

Tests stoped failing when changed to coping the template path to both destinations...

Should the link exist? I'm not sure, its just getting copied to root. If scripts append to the user, is it appropriate for root rc to reference it in the context of a dev container?

naturedamends · 2023-08-17T18:56:04Z

 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


I would take the meaning of -e and symlink to mean xor... not sure Samruddhi Khandale (@samruddhikhandale)

Samruddhi Khandale (samruddhikhandale) · 2023-08-17T21:52:29Z

+# 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


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 -

"features": { "common-utils": { "installZsh": true, } }

Then a user creates their devcontainer.json as 👇

{ "image": "X", "features": { "common-utils": { "installZsh": false } } }

"installZsh": false

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 install zsh but it doesn't mean that it should remove the already existing zsh. 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!

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

Or do you not like the file being removed at all?

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.

Hose about linking ~/.zshrc to .zshrc.devcontainer.

Then downstream can handle, or ln -sf?

Samruddhi Khandale (samruddhikhandale) · 2023-08-28T21:48:00Z

+# *********************************
+# ** Ensure config directory **
+# *********************************
+user_config_dir="${user_home}/.config"
+if [ ! -d "${user_config_dir}" ]; then
+    mkdir -p "${user_config_dir}"
+    chown ${USERNAME}:${group_name} "${user_config_dir}"
+fi
+


features/src/common-utils/main.sh

Lines 392 to 400 in 1724e14

if [ "${USERNAME}" = "root" ]; then

user_home="/root"

else

user_home="/home/${USERNAME}"

if [ ! -d "${user_home}" ]; then

mkdir -p "${user_home}"

chown ${USERNAME}:${group_name} "${user_home}"

fi

fi

configures user_home, hence, should this be added after that?

Yeah good catch. I want a clean home directory, so if im going to program this, I want it before any rc files. Not sure how much of a pain this is going to be

I came to the conclusion that you would not want any related flags in common-utils

Is the shell-setup feature going to be in features repo?
Then would it be prudent to add a flag to prevent any rc generation here

Thus you can enable the following to get equal functionality:
Common-utils rc false
Shell-setup shell zsh

I'm not familiar with any other fancy shells. Other than one I heard of that has a consistent output table format.

Would you consider adding a shell-setup to the features repo?

Sorry for bad grammar on tablet

Samruddhi Khandale (samruddhikhandale) · 2023-08-28T22:00:50Z

+            # 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/}"


Curious, why is this needed?

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
Or scripts appended to users linked template

Nvm. I will close pr.

Enable installOhMyZshConfig false for upstream images.

f5b5e6a

naturedamends requested a review from a team as a code owner August 13, 2023 20:32

naturedamends mentioned this pull request Aug 13, 2023

[common-utils]: Bug fix: Install zsh on an image previously built with "installZsh:false" #649

Merged

Update configure_zsh_no_template_second_step.sh

6e635a6

Samruddhi Khandale (samruddhikhandale) reviewed Aug 14, 2023

View reviewed changes

Comment thread src/common-utils/main.sh Outdated

Comment thread src/common-utils/main.sh Outdated

Comment thread src/common-utils/main.sh Outdated

Comment thread src/common-utils/main.sh

Comment thread test/common-utils/scenarios.json Outdated

Comment thread test/common-utils/scenarios.json

Correct testing and user file permissions.

f279594

naturedamends commented Aug 15, 2023

View reviewed changes

Comment thread src/common-utils/main.sh Outdated

Handle more edge cases.

b40bbd6

Update main.sh

431729f

Improve edge cases, clean up.

108ab53

naturedamends changed the title ~~WIP: Common-utils: Enable installOhMyZshConfig false for upstream images.~~ Common-utils: Enable installOhMyZshConfig false for upstream images. Aug 16, 2023

naturedamends changed the title ~~Common-utils: Enable installOhMyZshConfig false for upstream images.~~ WIP: Common-utils: Enable installOhMyZshConfig false for upstream images. Aug 16, 2023

naturedamends and others added 2 commits August 16, 2023 23:00

Update main.sh

4c68aee

Change testing and coding standards.

dc73b2d

naturedamends commented Aug 17, 2023

View reviewed changes

Remove old test.

3150f0d

naturedamends changed the title ~~WIP: Common-utils: Enable installOhMyZshConfig false for upstream images.~~ Common-utils: Enable installOhMyZshConfig false for upstream images. Aug 17, 2023

Samruddhi Khandale (samruddhikhandale) reviewed Aug 17, 2023

View reviewed changes

Remove removing files.

836f9e9

naturedamends changed the title ~~Common-utils: Enable installOhMyZshConfig false for upstream images.~~ Common-utils: various cleaning things from other pr. Aug 18, 2023

Lines to previous state.

1724e14

naturedamends mentioned this pull request Aug 22, 2023

Commonutils: chown: cannot access '/home/vscode/.zshrc': No such file or directory #662

Merged

Samruddhi Khandale (samruddhikhandale) reviewed Aug 28, 2023

View reviewed changes

naturedamends closed this Aug 29, 2023

	if [ "${USERNAME}" = "root" ]; then
	user_home="/root"
	else
	user_home="/home/${USERNAME}"
	if [ ! -d "${user_home}" ]; then
	mkdir -p "${user_home}"
	chown ${USERNAME}:${group_name} "${user_home}"
	fi
	fi

Uh oh!

Conversation

naturedamends commented Aug 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposal

failed proposal

Uh oh!

naturedamends commented Aug 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Samruddhi Khandale (samruddhikhandale) left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

naturedamends commented Aug 15, 2023

Uh oh!

Uh oh!

Samruddhi Khandale (samruddhikhandale) commented Aug 15, 2023

Uh oh!

naturedamends commented Aug 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

naturedamends Aug 17, 2023

Choose a reason for hiding this comment

Uh oh!

Samruddhi Khandale (samruddhikhandale) Aug 17, 2023

Choose a reason for hiding this comment

Uh oh!

naturedamends Aug 17, 2023

Choose a reason for hiding this comment

Uh oh!

Samruddhi Khandale (samruddhikhandale) Aug 17, 2023

Choose a reason for hiding this comment

Uh oh!

naturedamends Aug 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Samruddhi Khandale (samruddhikhandale) Aug 28, 2023

Choose a reason for hiding this comment

Uh oh!

naturedamends Aug 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Samruddhi Khandale (samruddhikhandale) Aug 28, 2023

Choose a reason for hiding this comment

Uh oh!

naturedamends Aug 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

naturedamends commented Aug 13, 2023 •

edited

Loading

naturedamends commented Aug 13, 2023 •

edited

Loading

naturedamends commented Aug 16, 2023 •

edited

Loading

naturedamends Aug 17, 2023 •

edited

Loading

naturedamends Aug 29, 2023 •

edited

Loading

naturedamends Aug 29, 2023 •

edited

Loading