Skip to content

Conversation

@akalipetis
Copy link
Member

No description provided.

Copilot AI review requested due to automatic review settings November 18, 2025 12:26
Copilot finished reviewing on behalf of akalipetis November 18, 2025 12:29
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds devcontainer configuration to enable development using Visual Studio Code devcontainers or GitHub Codespaces. The setup includes a Dockerfile based on PHP 7.4-cli and a devcontainer.json configuration that automatically runs composer install after container creation.

Key Changes

  • Added .devcontainer/devcontainer.json with basic configuration for the "Upsun CLI" devcontainer
  • Added .devcontainer/Dockerfile with PHP 7.4, Composer installation, and non-root user setup
  • Configured automatic dependency installation via postCreateCommand

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.

File Description
.devcontainer/devcontainer.json Defines devcontainer configuration with remote user and post-creation command
.devcontainer/Dockerfile Sets up PHP 7.4 environment with Composer and non-root user for development

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +4 to +25
RUN apt-get update && apt-get install -y \
zip \
unzip \
git \
&& rm -rf /var/lib/apt/lists/*

# Install Composer
RUN php -r "copy('https://getcomposer.org/installer', 'composer-setup.php');" \
&& php composer-setup.php \
&& mv composer.phar /usr/local/bin/composer \
&& php -r "unlink('composer-setup.php');" \
&& chmod +x /usr/local/bin/composer

# Create a non-root user
ARG USERNAME=upsun
ARG USER_UID=1000
ARG USER_GID=$USER_UID

RUN groupadd --gid $USER_GID $USERNAME \
&& useradd --uid $USER_UID --gid $USER_GID -m $USERNAME \
&& apt-get update \
&& apt-get install -y sudo \
Copy link

Copilot AI Nov 18, 2025

Choose a reason for hiding this comment

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

apt-get update is called twice unnecessarily (line 4 and line 24), which is inefficient and slows down image builds. Consider combining all apt-get installations into a single RUN command or removing the cleanup on line 8 and doing it after the final apt-get install on line 25.

Copilot uses AI. Check for mistakes.
&& echo $USERNAME ALL=\(root\) NOPASSWD:ALL > /etc/sudoers.d/$USERNAME \
&& chmod 0440 /etc/sudoers.d/$USERNAME

USER $USERNAME
Copy link

Copilot AI Nov 18, 2025

Choose a reason for hiding this comment

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

The Dockerfile is missing a WORKDIR directive. The main project Dockerfile sets WORKDIR /home/psh/legacy-cli (line 23 in Dockerfile), which defines where the code will be mounted and where commands will run. Consider adding a similar WORKDIR for the devcontainer, e.g., WORKDIR /home/upsun/workspace or an appropriate path.

Suggested change
USER $USERNAME
USER $USERNAME
# Set the working directory
WORKDIR /home/upsun/workspace
# Ensure the workspace directory exists
RUN mkdir -p /home/upsun/workspace

Copilot uses AI. Check for mistakes.
Comment on lines +10 to +15
# Install Composer
RUN php -r "copy('https://getcomposer.org/installer', 'composer-setup.php');" \
&& php composer-setup.php \
&& mv composer.phar /usr/local/bin/composer \
&& php -r "unlink('composer-setup.php');" \
&& chmod +x /usr/local/bin/composer
Copy link

Copilot AI Nov 18, 2025

Choose a reason for hiding this comment

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

The Composer installation lacks signature verification, which is a security concern. The existing Dockerfile in the repository uses a more secure installation method that verifies the installer signature. Consider using the same approach from docker/install_composer.sh or at minimum verify the installer signature before running it.

Suggested change
# Install Composer
RUN php -r "copy('https://getcomposer.org/installer', 'composer-setup.php');" \
&& php composer-setup.php \
&& mv composer.phar /usr/local/bin/composer \
&& php -r "unlink('composer-setup.php');" \
&& chmod +x /usr/local/bin/composer
# Install Composer (with signature verification)
RUN set -eux; \
EXPECTED_SIGNATURE="$(php -r 'copy("https://composer.github.io/installer.sig", "php://stdout");')" ; \
php -r "copy('https://getcomposer.org/installer', 'composer-setup.php');" ; \
ACTUAL_SIGNATURE="$(php -r "echo hash_file('SHA384', 'composer-setup.php');")" ; \
if [ "$EXPECTED_SIGNATURE" != "$ACTUAL_SIGNATURE" ]; then \
echo 'ERROR: Invalid installer signature' >&2; \
rm -f composer-setup.php; \
exit 1; \
fi; \
php composer-setup.php; \
mv composer.phar /usr/local/bin/composer; \
php -r "unlink('composer-setup.php');"; \
chmod +x /usr/local/bin/composer

Copilot uses AI. Check for mistakes.
unzip \
git \
&& rm -rf /var/lib/apt/lists/*

Copy link

Copilot AI Nov 18, 2025

Choose a reason for hiding this comment

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

The devcontainer Dockerfile is missing the pcntl PHP extension that is installed in the main project Dockerfile (see line 11 in Dockerfile). This extension is likely required for the CLI to function properly. Add RUN docker-php-ext-install -j$(nproc) pcntl after installing system dependencies.

Suggested change
# Install pcntl extension (required for CLI)
RUN docker-php-ext-install -j$(nproc) pcntl

Copilot uses AI. Check for mistakes.
RUN apt-get update && apt-get install -y \
zip \
unzip \
git \
Copy link

Copilot AI Nov 18, 2025

Choose a reason for hiding this comment

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

Missing system dependencies that are present in the main project Dockerfile: wget and openssh-client (see lines 7-8 in Dockerfile). These are likely needed for the CLI's functionality, such as SSH access to environments and downloading resources. Add them to the apt-get install command on lines 4-8.

Suggested change
git \
git \
wget \
openssh-client \

Copilot uses AI. Check for mistakes.
@@ -0,0 +1,8 @@
{
"name": "Upsun CLI",
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think let's figure this out when we merge to upsun/cli and update to PHP 8.4 or 8.5

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants