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

Update the dependencies (and sort out the resulting mess) #2246

Open
wants to merge 20 commits into
base: master
Choose a base branch
from

Conversation

joonas-somero
Copy link
Member

@joonas-somero joonas-somero commented Sep 12, 2024

Proposed changes

Title.

Deployed at https://demo-sidebars-docs-csc-development.2.rahtiapp.fi/ (fork)

TODO:

  • A What's new entry
  • Update dependecies for Preview
  • Announcement (please feel free to edit)

@joonas-somero joonas-somero added the on hold This is waiting for some specific event before it can be merged label Sep 13, 2024
@joonas-somero joonas-somero marked this pull request as ready for review September 13, 2024 05:38
@joonas-somero
Copy link
Member Author

I suppose this should wait for Preview to support the new dependencies.

Also, take the opportunity to move them into Allas
E.g. when building locally with Podman or Docker
Dockerfile Outdated
Comment on lines 30 to 40
RUN if [ ! -d ".git" ]; then \
git clone --no-checkout https://github.com/$repo_org/$repo_name git_folder && \
mv git_folder/.git . && \
rm -r git_folder && \
git reset HEAD --hard && \
git checkout -f $repo_branch
git checkout -f $repo_branch; fi

RUN pip3 install --no-cache-dir -r requirements.txt && \
bash scripts/generate_alpha.sh && \
RUN bash scripts/generate_alpha.sh && \
bash scripts/generate_by_system.sh && \
bash scripts/generate_new.sh && \
bash scripts/generate_glossary.sh && \
Copy link
Member

Choose a reason for hiding this comment

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

Why not join these two RUNs?

If they are joined and the .git directory is deleted as a last step, there will be only one layer and it will be smaller.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've now joined them. For getting the missing .git directory, I was thinking

RUN if [ ! -d ".git" ]; then \
    git clone --bare https://github.com/$repo_org/$repo_name .git && \
    git init && \
    git switch --force $repo_branch; fi && \
    bash scripts/generate_alpha.sh && \
    bash scripts/generate_by_system.sh && \
    bash scripts/generate_new.sh && \
    bash scripts/generate_glossary.sh && \
    mkdocs build -d /usr/share/nginx/html

I don't know if it's any quicker, but it is more readable.

@lvarin
Copy link
Member

lvarin commented Sep 13, 2024

I checked the changes but not the CSS, because I do not know enough. Maybe @jemaltahir can comment on those changes.

Looks good to me, only the small comment about the Dockerfile.

@jemaltahir
Copy link
Member

I am reviewing the CSS part.

Copy link
Member

@jemaltahir jemaltahir left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
on hold This is waiting for some specific event before it can be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants