fix: update Dockerfile with python and .gyp to support install and run of Glee project in container #1208
Conversation
Add python and .gyp to support install and run of Glee project in container
eelcofolkertsma
requested review from
Souvikns,
magicmatatjahu,
derberg and
asyncapi-bot-eve
as code owners
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Welcome to AsyncAPI. Thanks a lot for creating your first pull request. Please check out our contributors guide useful for opening a pull request.
Keep in mind there are also other channels you can use to interact with AsyncAPI community. For more details check out this issue.
eelcofolkertsma
changed the title
eelcofolkertsma
changed the title
Revert Dockerfile, add composeGlee
|
I have developed an alternative solution to establish a container with running Glee server. The change to Dockerfile has been undone. Instead I have added a file composeGlee.yaml for invocation with 'docker compose -f composeGlee.yaml up' Docker Compose
Known deficiency: permissions on new glee project are "root" write only. I have solved with chown on my own machine but this can be done more nicely |
bring back python and .gyp in Dockerfile
Used for test only, fix image reference
|
In consultation with Lukasz
Running container supports studio and start of glee server. This was tested with docker exec -it Note: .github/workflows folder contains a flow to publish docker image to Docker hub on new release. Rather confusing this workflow speaks about an Ubuntu image. I have no experience in workflows so I cannot track the error |
compose.yaml
Outdated
| @@ -0,0 +1,16 @@ | |||
| version: '3' | |||
There was a problem hiding this comment.
Not sure why we need a compose.yaml committed in the repo.
If it's needed I suggest to rename it to docker-compose.yaml
To be confirmed by current maintainers
There was a problem hiding this comment.
I do use the file in testing the viability of docker container (studio, glee), but test is far from trivial. I am very open to other test approach
There was a problem hiding this comment.
File has been renamed as per suggestion and moved to location ./test/manual
Comment in Dockerfile has been adjusted accordingly (instructions for testing)
test/manual/docker-compose.yaml
Outdated
| @@ -0,0 +1,16 @@ | |||
| version: '3' | |||
There was a problem hiding this comment.
why do you think it is needed to keep in the repo if in the end it is not used in any automated tests? what is the value of this file?
There was a problem hiding this comment.
It seems current image is not tested at all, not automated nor manual. Example: the ARG for cli version that I removed as dysfunctional. I guess that behavior has been around for many months.
Than a manual test is better than nothing.
I lack skill to develop an automated test for the image, but fine with me to drop the manual test resource and raise an issue to validate the docker image in some automated way.
There was a problem hiding this comment.
On the ARG... I have pulled image from DockerHub and inspected version of asyncapi in resulting container. The ARG is ignored in building of image! Current version is 1 something, not 0.59 as requested per ARG. And this is good.
I can see value to manually install an older (trusted) version of CLI, but for the image you would expect latest and greatest version. Docker Hub has the older versions as well as in docker pull asyncapi/cli:1.6.14
There was a problem hiding this comment.
oh, you're right, we missed it and did not add a simple docker build testing - lemme fix that, we have some generic workflows that I can pull in
There was a problem hiding this comment.
ok, we have now https://github.com/asyncapi/cli/blob/master/.github/workflows/if-docker-pr-testing.yml in this repo, lets update this PR
There was a problem hiding this comment.
ok, so automated tests are in place now
next
yes, we know the arg is ignored (overwritten) in release: https://github.com/asyncapi/cli/blob/master/.github/workflows/release-docker.yml#L42 so we always want to release a latest
the arg in docker file was added for a reason -> see the story in #675
does it explain all?
maybe instead of removing ARG ASYNCAPI_CLI_VERSION=0.59.1 we should add a comment for future, linking to the other PR I pointed out?
There was a problem hiding this comment.
You predict my thoughts. The ARG goes back in with comment that it is for manual image create only, github workflow overrides. #675 has lots of insider communication, perhaps a bit too much ;-)
|
Hi Lukasz, all is in place now: automated test on Docker image, python and .gyp in Docker image, and node:20 For your approval |
Dockerfile
Outdated
| rm -rf /var/lib/apt/lists/* && \ | ||
| rm /var/cache/apk/* | ||
|
|
||
| # Installing latest released npm package | ||
| RUN npm install --ignore-scripts -g @asyncapi/cli@"$ASYNCAPI_CLI_VERSION" | ||
| RUN npm install --ignore-scripts -g @asyncapi/cli |
There was a problem hiding this comment.
@eelcofolkertsma you added ASYNCAPI_CLI_VERSION back in line 4 but it is still missing here - basically not used
There was a problem hiding this comment.
OK, fixed that one too
If you use Dockerfile without change you get latest version of cli. If you add a version-value you get image for that version of cli
Tested OK in Github codespaces (default version=1.7.3, forced version=1.7.2) with docker build -t cli . and docker run cli --version
|
|
/rtm |
|
🎉 This PR is included in version 1.8.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Add python and .gyp to support install and run of Glee project in container
In follow up to #1207 bug
Glee install fails over python. I have followed best practice for Node images to patch Dockerfile with apk for python, .gyp and related stuff
Related issue(s)
Fixes #1207