-
Notifications
You must be signed in to change notification settings - Fork 148
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
Change the image #3597
Change the image #3597
Conversation
docker-compose-dev.yml
Outdated
@@ -42,7 +42,7 @@ services: | |||
|
|||
web: | |||
restart: "unless-stopped" | |||
image: cernopendata/static # Use this to make sure that COD3 Python-code image is built only once. | |||
image: registry.cern.ch/cernopendata/cernopendata-portal:latest |
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 comments:
-
This image does not contain JADE links in the header, the home page body and the footer. The
master
branch needs those. This should be easy to fix to have custom images forproduction
,'qa
andmaster
branches. -
More importantly, the Jinja template changes newer than the image production date are not displayed when developing locally for me, even though the local files are mounted dynamically into the container. This means that local developments related to adding new fields or amending output templates does not seem to be possible. E.g. try the new dataset semantics files information with this branch:
$ docker exec -i -t opendatacernch-web-1 cernopendata fixtures records --mode insert-or-replace -f cernopendata/modules/fixtures/data/records/cms-derived-pfnano-2016.json $ firefox http://127.0.0.1:5000/record/31300
-
For parity, the
docker-compose.yaml
file would have to be amended too, with instructions on how to build a custom image version, so that static images in the newly developed documentation pages (those after the image creation date) would show up locally. E.g try the new plot in the pile-up guide:$ docker exec -i -t opendatacernch-web-1 cernopendata fixtures docs --mode insert-or-replace -f /code/cernopendata/modules/fixtures/data/docs/cms-guide-pileup-simulation/cms-guide-pileup-simulation.json $ firefox http://127.0.0.1:5000/docs/cms-guide-pileup-simulation
-
Finally, the DEVELOPING guide instructions would have to be updated regarding
docker compose build
and friends, since they don't apply anymore, as they would still builddocker.io/cernopendata/static
etc.
The fix in #3594 works out of the box without having any of these problems.
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.
Thanks for checking. Let's check a different approach then. None of the comments above are valid anymore with this approach
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've tested the static image asset gathering use case described above, and the update fixes it. 👍
However I'm still seeing some issues, e.g. a build problem related to overwriting assets:
$ docker compose -f docker-compose-dev.yml build
...
9.858 Traceback (most recent call last):
9.858 File "/opt/invenio/var/instance/python/bin/cernopendata", line 33, in <module>
9.858 Collect static from blueprints
9.858 sys.exit(load_entry_point('cernopendata', 'console_scripts', 'cernopendata')())
9.858 File "/opt/invenio/var/instance/python/lib/python3.9/site-packages/click/core.py", line 1157, in __call__
9.859 return self.main(*args, **kwargs)
9.859 File "/opt/invenio/var/instance/python/lib/python3.9/site-packages/click/core.py", line 1078, in main
9.859 rv = self.invoke(ctx)
9.859 File "/opt/invenio/var/instance/python/lib/python3.9/site-packages/click/core.py", line 1688, in invoke
9.859 return _process_result(sub_ctx.command.invoke(sub_ctx))
9.859 File "/opt/invenio/var/instance/python/lib/python3.9/site-packages/click/core.py", line 1434, in invoke
9.859 return ctx.invoke(self.callback, **ctx.params)
9.859 File "/opt/invenio/var/instance/python/lib/python3.9/site-packages/click/core.py", line 783, in invoke
9.859 return __callback(*args, **kwargs)
9.859 File "/opt/invenio/var/instance/python/lib/python3.9/site-packages/click/decorators.py", line 33, in new_func
9.860 return f(get_current_context(), *args, **kwargs)
9.860 File "/opt/invenio/var/instance/python/lib/python3.9/site-packages/flask/cli.py", line 357, in decorator
9.860 return __ctx.invoke(f, *args, **kwargs)
9.860 File "/opt/invenio/var/instance/python/lib/python3.9/site-packages/click/core.py", line 783, in invoke
9.860 return __callback(*args, **kwargs)
9.860 File "/opt/invenio/var/instance/python/lib/python3.9/site-packages/invenio_assets/cli.py", line 24, in collect
9.860 current_assets.collect.collect(verbose=verbose)
9.860 File "/opt/invenio/var/instance/python/lib/python3.9/site-packages/flask_collect/collect.py", line 50, in collect
9.860 return storage.run()
9.860 File "/opt/invenio/var/instance/python/lib/python3.9/site-packages/flask_collect/storage/link.py", line 49, in run
9.860 os.symlink(f, destination)
9.860 FileExistsError: [Errno 17] File exists: '/opt/invenio/var/instance/python/lib/python3.9/site-packages/invenio_previewe
r/static/css/pdfjs/viewer.css' -> '/opt/invenio/var/instance/static/css/pdfjs/viewer.css'
9.876 /usr/lib64/python3.9/site-packages/XRootD/client/finalize.py:46: DeprecationWarning: Importing 'itsdangerous.json' is d
eprecated and will be removed in ItsDangerous 2.1. Use Python's 'json' module instead.
9.876 if isinstance(obj, File) and obj.is_open():
9.876
------
failed to solve: process "/bin/sh -c scripts/create-instance.sh" did not complete successfully: exit code: 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.
Thanks for pointing this out. I have the feeling that master has the same issue since this commit. Could you please confirm that the patch in scripts/create_instance fixes it for you as well?
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.
@psaiz Can you please rebase against the latest master
? I can recheck afterwards all the above use cases.
b037631
to
88b8b1d
Compare
ebcb930
to
1271202
Compare
Closing in favor of #3590 |
No description provided.