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

Bump mark to 9.11.1 #16

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
Open

Bump mark to 9.11.1 #16

wants to merge 15 commits into from

Conversation

ramirezalfredo
Copy link

@ramirezalfredo ramirezalfredo commented Jan 15, 2024

  • Bump mark version to 9.11.1
  • Docker actions must be run by the default Docker user (root).
  • Do not use the USER instruction in your Dockerfile, because you won’t be able to access the GITHUB_WORKSPACE directory.
  • Removed Chrome installation, using chromedp/headless-shell:latest image instead.

@ramirezalfredo ramirezalfredo requested a review from a team as a code owner January 15, 2024 22:01
Comment on lines +23 to +24
ENTRYPOINT ["/usr/bin/dumb-init", "--"]
CMD ["python3", "/app/mark2confluence/main.py"]
Copy link
Member

Choose a reason for hiding this comment

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

I don't think there is needs of an init script since the python script doesn't need to run as daemon here

Choose a reason for hiding this comment

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

I think I agree about not changing the entrypoint

WORKDIR /app
ENV PYTHONPATH /app
ENV DOC_PREFIX /github/workspace/
ENV LOGURU_FORMAT "<lvl>{level:7} {message}</lvl>"
USER 1001:1001
Copy link
Member

Choose a reason for hiding this comment

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

This is a quite big security concern, it will require an explicit exception to run it on our clusters. (Kyverno will prevent it to run at all, afaik)

Copy link

Choose a reason for hiding this comment

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

Agreed. How come we have to perform this change now? Did it work before with the unprivileged user?
The doc seems to go in the direction of that change though: https://docs.github.com/en/actions/creating-actions/dockerfile-support-for-github-actions#user

Choose a reason for hiding this comment

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

no not really. I had to add this to my action to get it to work which is awful

   - name: chown files to UID 1001
     shell: bash
     run: sudo chown -R 1001:1001 *

Dockerfile Outdated Show resolved Hide resolved
Dockerfile Outdated

FROM python:3-slim
FROM chromedp/headless-shell:latest
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure about this change instead of copy from builder...you are moving from 156MB of base image to more 285MB this will have a sensible impact on our build times
image

Choose a reason for hiding this comment

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

Looks like this comment got marked as outdated. I agree with mbovo on this. No need to use a different/larger container.

@eric-engberg
Copy link

There's been no progress for 3 weeks on this. Should I open a new PR with just upgrading the version of mark?

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.

4 participants