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.

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

remove about page and its tests #1772

Merged
merged 5 commits into from
May 10, 2023
Merged

remove about page and its tests #1772

merged 5 commits into from
May 10, 2023

Conversation

smoqadam
Copy link
Contributor

No description provided.

@smoqadam smoqadam requested a review from nlisgo April 11, 2023 12:33
@nlisgo
Copy link
Member

nlisgo commented Apr 12, 2023

There is one page we need to preserve on the journal and that is https://elifesciences.org/about/people. Apologies that it wasn't clearer but it was in the description of the issue :/

We also can remove some entries in the routing.yml. Although we are removing all but the about-people some of the routes are still needed because they are referenced in navigation or other pages.

Let's have a brief chat about how to proceed.

Copy link
Member

@nlisgo nlisgo left a comment

Choose a reason for hiding this comment

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

Needs a slightly different approach.

@smoqadam
Copy link
Contributor Author

The pipeline is failing because the Debian stretch has been archived.
https://lists.debian.org/debian-devel-announce/2023/03/msg00006.html

I've fixed that in my local machine by telling Debian to use archive.debian.org for updating packages.
I added the following lines in Dockerfile.npm, Dockerfile.assets_builder, and Dockerfile.critical_css which are using node:

RUN echo "deb http://archive.debian.org/debian/ stretch main non-free contrib" > /etc/apt/sources.list
RUN echo "deb-src http://archive.debian.org/debian/ stretch main non-free contrib" >> /etc/apt/sources.list
RUN echo "deb http://archive.debian.org/debian-security/ stretch/updates main non-free contrib" >> /etc/apt/sources.list
RUN echo "deb-src http://archive.debian.org/debian-security/ stretch/updates main non-free contrib" >> /etc/apt/sources.list

I'm not sure if that's the best way. @lsh-0 could you please look into this?

Thank you

cc: @nlisgo @scottaubrey

@lsh-0
Copy link
Contributor

lsh-0 commented May 1, 2023

I wasn't able to look at this today, it's on my plate for tomorrow.

@lsh-0
Copy link
Contributor

lsh-0 commented May 2, 2023

I'm not sure if that's the best way. @lsh-0 could you please look into this?

After a quick inspection of the repos it looks like journal and pattern-library may still depend on an older version of nodejs:

I recommend you upgrade to nodejs 16 lts, probably in a separate branch of work.

@lsh-0
Copy link
Contributor

lsh-0 commented May 2, 2023

This might help you get started: #1777

@lsh-0
Copy link
Contributor

lsh-0 commented May 3, 2023

@lsh-0 you are correct that the upgrade is long overdue. I'm unclear why this is failing now. Could you help to shed some light on that please?

Sorry, jumped to the solution without showing my working out. Essentially, these Dockerfiles are extending an old Docker image.

For example, in Dockerfile.npm we have these in the header:

ARG node_version
FROM node:${node_version} as npm

and that node_version is being set during the call to docker-compose build to the NODE_VERSION value in the .env file:

services:
    [...]
    npm:
        build:
            context: .
            dockerfile: Dockerfile.npm
            args:
                node_version: ${NODE_VERSION}
        image: elifesciences/journal_npm:${IMAGE_TAG}

which looks like:

NODE_VERSION=6.17.0-stretch

So now FROM node:${node_version} as npm becomes FROM node:6.17.0-stretch as npm.

Taking a peek at the node:6.17.0-stretch image on Dockerhub we can see it hasn't been updated in a while:

Screenshot at 2023-05-03 11-44-56

In the meantime the world has moved on and, as @smoqadam says above, the package repositories have been deleted/replaced.

One way of getting around that without changing the image is replacing the list of repositories in every instance where this old image is being used, running apt-update, etc. @smoqadam says it works for him, so that might be one way to proceed.

A better way to proceed would be to update the Docker image you depend on to something more recent that also provides nodejs 6. Unfortunately you're already on the most recent image version that provides nodejs 6.

You could replace this node image providing nodejs 6 with your own image where you build and configure a version of nodejs to suit your needs, rather like I do here for the non-containerised infrastructure, or, you could upgrade to a version of node that the more recently built images provide.

I recommend upgrading to nodejs 16 lts and I opened a PR that switches journal to it but obviously with such a large leap in versions there will be other problems to iron out, so it's just a suggestion to get started.

Hope this helps.

@scottaubrey
Copy link
Member

@nlisgo Also worth saying, the reason it has stopped now is that Debian moved stretch packages out of the main repos to archive (as the updated repos fix proves) very recently: https://lists.debian.org/debian-devel-announce/2023/03/msg00006.html

@nlisgo
Copy link
Member

nlisgo commented May 3, 2023

@lsh-0 @scottaubrey thanks for the explanation. I felt silly asking it because @smoqadam mentioned the problem in a previous comment. I suggest that we find a way to rely on the archive for now so that we are unblocked on journal deployments and that we prioritise upgrading to a more recent image of nodejs.

@scottaubrey
Copy link
Member

@lsh-0 @scottaubrey thanks for the explanation. I felt silly asking it because @smoqadam mentioned the problem in a previous comment. I suggest that we find a way to rely on the archive for now so that we are unblocked on journal deployments and that we prioritise upgrading to a more recent image of nodejs.

Ha! I missed that too, sorry for the noise everyone :)

I agree, adding the workaround unblocks this work in the short term, but we really must prioritise getting these out of date dependencies up to date.

@nlisgo
Copy link
Member

nlisgo commented May 3, 2023

I agree, adding the workaround unblocks this work in the short term, but we really must prioritise getting these out of date dependencies up to date.

PR in play to buy us the time to upgrade while not blocking journal deployments: #1778

We will need to do something similar on pattern-library.

@nlisgo
Copy link
Member

nlisgo commented May 3, 2023

@smoqadam this is unblocked for now but we will be discussing the appropriate priority of upgrading the version of node needed for critical_css and assets_builder

@nlisgo nlisgo self-requested a review May 3, 2023 12:59
@smoqadam smoqadam merged commit 7746a1d into develop May 10, 2023
@smoqadam smoqadam deleted the remove-about branch May 10, 2023 09:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants