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
MDBF-467: Add WordPress builder #54
Conversation
2bd0f37
to
72bec24
Compare
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.
Some shellcheck pb that make me think we should merge #7...
Rest looks good (appart from the arch feature in CI containers).
Also, I kind of like the idea that we try to test our containers before pushing in the registry, I am not sure how we could do it though but that would be good to have...
.github/workflows/eco_containers.yml
Outdated
| include: | ||
| - containerfile: wordpress_phpunit_test_runner.Dockerfile | ||
| tag: wordpress_phpunit_test_runner | ||
| platforms: amd64 |
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.
Probably not necessary (or should we support other arch in the future?)
| steps: | ||
| - uses: actions/checkout@v3 | ||
| # - name: Set up QEMU | ||
| # uses: docker/setup-qemu-action@v2 |
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.
same comment as for arch support
.github/workflows/eco_containers.yml
Outdated
| tags: ${{ matrix.tag }} | ||
| containerfiles: ${{ env.WORKDIR }}/${{ matrix.containerfile }} | ||
| context: ${{ env.WORKDIR }} | ||
| aches: ${{ matrix.arches }} |
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.
Not needed and probably typo.
|
|
||
| # https://github.com/WordPress/phpunit-test-runner/blob/master/README.md#0-requirements | ||
|
|
||
| # hadolint ignore=DL3008 |
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.
Not sure why this is needed, see: https://github.com/MariaDB/buildbot/blob/main/.hadolint.yaml#L5
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.
because the ignore file wasn't passed though to the hadolint 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.
ok you probably want to copy it in the workdir before the check then? Not sure what the best option is but we probably do not want to duplicate ignore lists?
|
|
||
| RUN bash -c 'set -o pipefail ; curl -o - https://raw.githubusercontent.com/nvm-sh/nvm/v0.39.1/install.sh | bash' | ||
| # hadolint ignore=DL3059 | ||
| RUN bash -c '. "$NVM_DIR/nvm.sh" && nvm install 14.15.0' |
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.
Any reason to multiply layers here (why not consolidate those RUN in one?)
scripts/docker-library-build.sh
Outdated
| pkgver=ubu2204 | ||
| ;; | ||
| 2004-deb-autobake) | ||
| base=focal |
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.
not used (shellcheck)
scripts/docker-library-build.sh
Outdated
| ;; | ||
| esac | ||
|
|
||
| buildernamebase=${buildername#*-} |
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.
not used (shellcheck)
scripts/docker-library-manifest.sh
Outdated
| # | ||
|
|
||
| buildmanifest() { | ||
| base=$1 |
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.
not used
c720bd7
to
e41d0b5
Compare
Restructure docker_library builder to separate steps with the WordPress builder triggered in the middle.
e41d0b5
to
8a7c6f3
Compare
Restructure docker_library builder to separate steps with the WordPress builder triggered in the middle.