Skip to content
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

Jump to bottom

Finetune the Dockerfile for Stepup builds #4

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Finetune the Dockerfile for Stepup builds #4

wants to merge 2 commits into from

Conversation

MKodde
Copy link
Member

@MKodde MKodde commented Jan 25, 2022 •
edited
Loading

This is the continuation of #3 That PR wasn't reviewed, and that should be done in this effort.

What changed here is:

  1. NVM is installed in a directory that was not yet created. Took care of that
  2. Node 10 is installed by default
  3. Yarn was installed

Using docker run, this container exposes php, yarn and composer

@MKodde MKodde force-pushed the feature/finetune-stepup-build-container branch 3 times, most recently from 23e185e to 6f912d8 Compare January 25, 2022 12:42
@MKodde MKodde requested a review from quartje January 25, 2022 12:45
@MKodde MKodde force-pushed the feature/finetune-stepup-build-container branch 4 times, most recently from 8a8fb6b to 6fd8ef8 Compare January 25, 2022 13:02
quartje
quartje reviewed Jan 25, 2022
View reviewed changes
Copy link
Contributor

@quartje quartje left a comment

Choose a reason for hiding this comment

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

Nice work. I like the idea of a general build container that is able to build all OpenConext projects.

I'd be nice if we don't have to put specific version numbers in this container, that makes it quickly outdated

.github/workflows/build-php-stepup.yml
@@ -17,7 +17,7 @@ jobs:
username: ${{ github.repository_owner }}
password: ${{ secrets.GITHUB_TOKEN }}

- name: Build the Apache container and push to GitHub Packages
- name: Build the Stepup build container and push to GitHub Packages
Copy link
Contributor

Choose a reason for hiding this comment

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

Would this be usable for EB as well? Makes maybe sense to have only one container for all build purposes

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess this should work with EB just fine. Certainly opportune to test this!

Copy link
Contributor

Choose a reason for hiding this comment

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

One container to rule them all!

docker/php-build-stepup/Dockerfile
@@ -20,11 +17,17 @@ ENV NVM_DIR /usr/local/nvm
ENV NODE_VERSION 10

# Install nvm with node and npm
RUN curl -o- https://raw.githubusercontent.com/nvm-sh/nvm/v0.37.2/install.sh | bash
Copy link
Contributor

Choose a reason for hiding this comment

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

Alltough this syntax works, it's more common to put && \ at the end of the line and start the newline with the next command

docker/php-build-stepup/Dockerfile
RUN curl -o- https://raw.githubusercontent.com/nvm-sh/nvm/v0.37.2/install.sh | bash
&& source $NVM_DIR/nvm.sh \
RUN mkdir $NVM_DIR \
&& curl -o- https://raw.githubusercontent.com/creationix/nvm/v0.39.0/install.sh | bash \
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there no more generic way than a hardcoded version to download?

Copy link
Member Author

Choose a reason for hiding this comment

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

I will look if they provide a latest or stable variety.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sadly there is no possibility to do this at this point.

docker/php-build-stepup/Dockerfile Outdated
# Yarn
&& npm install --global yarn \
# Link Node and Yarn to be available using Docker Run
&& ln -s /usr/local/nvm/versions/node/v10.24.1/bin/node /usr/local/bin/node \
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there no more generic way than a hardcoded version link? This will get outdated quickly

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I ended up with exporting the node version to an env-var. And using that instead. That should work out

quartje
quartje reviewed Jan 25, 2022
View reviewed changes
docker/php-build-stepup/Dockerfile
@@ -1,8 +1,5 @@
FROM php:7.2
Copy link
Contributor

Choose a reason for hiding this comment

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

would php-cli be enough?

Copy link
Member Author

Choose a reason for hiding this comment

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

Giving that a shot, It should be..

@MKodde MKodde force-pushed the feature/finetune-stepup-build-container branch 4 times, most recently from ee41084 to 9989a94 Compare January 25, 2022 14:52
@MKodde MKodde force-pushed the feature/finetune-stepup-build-container branch from 9989a94 to d23942e Compare January 25, 2022 14:52
@MKodde
Remove the symlinks
e5fcd0b 
We will be using the binaries from a bash standpoint, no need to expose
these for us them using docker run
@MKodde MKodde force-pushed the feature/finetune-stepup-build-container branch 2 times, most recently from 89ade2b to e5fcd0b Compare January 26, 2022 06:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@quartje quartje quartje left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@MKodde @quartje