-
Notifications
You must be signed in to change notification settings - Fork 2
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
Import docker examples #1
Import docker examples #1
Conversation
91413de
to
83ffe1c
Compare
Signed-off-by: Marc Dumais <marc.dumais@ericsson.com>
Resolves #908 Signed-off-by: Francesco Robino <francesco.robino@ericsson.com>
Perform a side (minor) 'yarn upgrade' to keep the yarn.lock fully aligned herein. This resulting yarn.lock is then based on going from clean to having run the yarn command locally a few times. Base this uplifted-to Theia version on community [1]'s below. Add a clarifying step in README so the yarn commands work locally. Fix the rebuild:browser scripting so it works with this and versus electron's. [1] https://theia-ide.org/releases/ Signed-off-by: Marco Miller <marco.miller@ericsson.com>
Perform a 'yarn upgrade' to keep the yarn.lock fully aligned. Base this uplifted-to Theia version on community [1]'s below. Fixed compilation error by making constructors of injectable classes public. Align to @postContruct changes of new inversify version [2]. Use Theia's re-export dependencies for inversify according to [3] Change electron-main location to ../src-gen/backend/ instead of ../src-gen/frontend due to breaking change described here [4] [1] https://theia-ide.org/releases/ [2] https://github.com/eclipse-theia/theia/blob/master/doc/Migration.md#inversify-60 [3] https://github.com/eclipse-theia/theia/blob/master/packages/core/README.md#re-exports [4] https://github.com/eclipse-theia/theia/blob/master/CHANGELOG.md#breaking_changes_1.39.0 Signed-off-by: Bernd Hufmann <bernd.hufmann@ericsson.com> Uplift Theia dependencies from v1.40.1 to 1.43.1 Perform a 'yarn upgrade' to keep the yarn.lock fully aligned. Base it on Theia community release [1] below. [1] https://theia-ide.org/releases/ Signed-off-by: Bernd Hufmann <bernd.hufmann@ericsson.com>
Before this commit the docker example was not properly building. The issue was introduced when moving from Theia 1.3x to 1.4x, where an extra step in the build process was introduced (i.e. running "yarn" was not enough, an extra "theia" command was needed). After this commit: - the docker example is building again - the docker image generated is stable and reproducible, not based on "next" - the docker image is much smaller thanks to a multistage build Signed-off-by: Francesco Robino <francesco.robino@ericsson.com>
Build the docker example image Signed-off-by: Marc Dumais <marc.dumais@ericsson.com> Update to node 18 Also update the required yarn version, that's somewhat related, aligning it to what's used in upstream Theia. Signed-off-by: Marc Dumais <marc.dumais@ericsson.com>
A few small changes that make the example docker image smaller and build a bit faster: - Build the theia trace viewer app/appliance in "production" mode (vs development) - Have yarn remove the installed "devDependencies" after the build ("yarn --production") - adapt docker-entrypoint.sh to start the app directly rather than through @theia/cli - Remove redundant "theia build" triggered in "prepare" package.json scripts entry - it's also done in Dockerfile Signed-off-by: Marc Dumais <marc.dumais@ericsson.com>
5fa1b63
to
75f8c5e
Compare
… a few testcases Create a "trace viewer app" from the generic @theia/playwright "theiaApp" and add a "trace explorer view". Enhance test suite with some related test cases. Signed-off-by: Marc Dumais <marc.dumais@ericsson.com>
Some views like statistics view have features to right-click on the entries and select time ranges, but this feature requires some Node.js-specific features like "process", which is missing for the docker example. Fixes #1066 Signed-off-by: Siwei Zhang <siwei.zhang@ericsson.com>
Add an example showing how to create a docker image that contains a Theia-based application with the vscode-trace-extension. Signed-off-by: Francesco Robino <francesco.robino@ericsson.com>
75f8c5e
to
6d39fd9
Compare
Quick comment. I have a few more images to contribute. One of them presents how to use the vscode-trace-extension in a vscode web application. The current naming of folders (theia-extension and vscode-extension) is a bit too vague to give a quick understanding of what the image is. For example, is "vscode-extension" the vscode-trace-extension running in a vscode app or in a theia app? Do you think would be worth to rename the folders using more details (e.g. theia-trace-ext-on-theia or theia-with-theia-trace-ext or similar)? Otherwise I can I differentiate my upcoming "vscode-trace-extension in vscode app" image? |
@frallax Nice! thanks for having a look at the PR - I will think about it a bit and propose more meaningful app folder names in an update to this PR. |
e328641
to
36508a1
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.
after removing the line that copies the yarn.lock (theia-app-theia-ext) the build continues, but fails later on:
#0 761.9 ERROR in ./node_modules/@inversifyjs/core/lib/esm/index.js 6:0-86
#0 761.9 Module not found: Error: Can't resolve './metadata/models/ClassElementMetadataKind' in '/app/tte/node_modules/@inversifyjs/core/lib/esm'
#0 761.9 Did you mean 'ClassElementMetadataKind.js'?
#0 761.9 BREAKING CHANGE: The request './metadata/models/ClassElementMetadataKind' failed to resolve only because it was resolved as fully specified
README.md
Outdated
|
||
### Docker examples | ||
|
||
Easy to consume _trace viewer appliance_ Docker images, thatt you can build yourself. For more details look under folder [docker](./docker) |
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.
thatt -> that
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.
Fixed
# Build the browser theia-trace-extension application | ||
COPY ${APP_SUBFOLDER}/package.json /app/tte/ | ||
COPY ${APP_SUBFOLDER}/webpack.config.js /app/tte/ | ||
COPY yarn.lock /app/tte/ |
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.
This line causes an error for me when runnig the build from this directory:
ERROR [build 5/7] COPY yarn.lock
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.
I have refactored the layout of the Docker example vs their theia applications and other resources. The Dockerfile(s ) are now in the "docker root" (<repo>/docker) and so I think it will be much clearer that the images should be built from that folder.
I have also much improved the READMEs.
docker/theia-app-theia-ext/README.md
Outdated
Try adding `--network host` in case of build failures related to debian packages retrieval: | ||
|
||
```bash | ||
docker build -t tate . |
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.
This doesn't work anymore. The build has to be done from the parent directory using the yarn command.
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.
This trick is still valid, but unfortunately did not help in that specific case. The build instructions have been migrated to the "docker" folder README and adjusted, which I think should help avoid this issue.
docker/theia-app-theia-ext/README.md
Outdated
the _tate_ image: | ||
|
||
```bash | ||
docker run --name tate-1 tate |
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.
After building the images from the parent directory, I went to this directory and and tried this command. I get the following error message:
Unable to find image 'tate:latest' locally
docker: Error response from daemon: pull access denied for tate, repository does not exist or may require 'docker login': denied: requested access to the resource is denied.
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.
The faulty instructions have been migrated to the parent folder README and adjusted. Can you please retry?
cadd930
to
8400a31
Compare
This was an interesting case. As long as the I think this shows one aspect of the value gained by maintaining a |
@bhufmann thanks for the review. I think your comments have been addressed by my last push. They have inspired me to refactor the layout of the "docker" folder and sub-folders, moving the Dockerfiles in the "docker" folder and leaving the image-specific resources in the subfolders. I think it's clearer this way, and hopefully you will agree. |
|
||
```bash | ||
docker run --name tv-tate-1 --network="host" -e TRACE_SERVER_URL=172.17.0.2:8080/tsp/api tv-tate | ||
``` |
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.
When I run this line after having run command in line 51 above I get the following error:
docker: Error response from daemon: Conflict. The container name "/tv-tate-1" is already in use by container "bdc7e40b3c61d60538b54e2842c34a66e9641d00b17511096d01d7db81e4782f". You have to remove (or rename) that container to be able to reuse that name.
It would be good to mention that the previous has to removed and provide users with the command for that for people that are not so familiar with docker.
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.
I have added a section at the bottom of the readme: "Docker tips and tricks", in which I gave a few ways to deal with this error, as well as a few other tips about other things.
Since this is the first "docker run" I have also specifically added an alternative command that discards the container after the run, as an example.
a44603c
to
22ba680
Compare
@bhufmann I think I have addressed your last remaining review comment. |
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.
Looks good to me. Thanks for providing this.
Also: - update them to use the "previous latest" Theia community release, v1.49.1 - move docker example using the Trace Viewer for VS Code - rename docker examples sub-folders - rename example-package.json to package.json Signed-off-by: Marc Dumais <marc.dumais@ericsson.com>
22ba680
to
15898f1
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.
LGTM. Thanks!
Thanks for the review @bhufmann ! |
This PR kick-starts this new repository.:
yarn.lock
. When building either one in its docker container, theyarn.lock
is used, insuring that curated npm dependencies/versions are pulled, and avoiding potential bad surprises.Note: when "importing" the two docker examples and the playwright test suite, care has been taken to keep important git commits that contributed to them, including preserving authorship. This makes for a slightly more messy git history in this repo here, but we believe it's a worthy trade-off. For more generic imported content, like root CONTRIBUTING file, I just copied-over and adapted, without preserving their original git history. Let me know during review if you do not agree with this.
Notes: