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

Add Dockerfile without web renderer #344

Merged
merged 2 commits into from
Jan 10, 2024

Conversation

wkozyra95
Copy link
Member

No description provided.

@wkozyra95 wkozyra95 force-pushed the @wkozyra95/docker-image-variants branch from 88bec60 to 9f7460c Compare January 8, 2024 15:03
Base automatically changed from @wkozyra95/add-env-config to master January 9, 2024 08:45
@wkozyra95 wkozyra95 force-pushed the @wkozyra95/docker-image-variants branch from 9f7460c to d089625 Compare January 9, 2024 12:31

- uses: hadolint/[email protected]
with:
dockerfile: build-tools/docker/full.Dockerfile

- name: Build image
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'm building only slim here, I'm wondering whether it make sense to build both. Any opinions?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't really know. If we're not using too much of the CI time then maybe we should build both versions, but I don't know how to check these limits/the spending

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 don't think there are limits for public repositories. My concern was about witing for CI, but this job is not always run, so maybe it make sense to run both

@wkozyra95 wkozyra95 force-pushed the @wkozyra95/docker-image-variants branch from d089625 to 9fe8523 Compare January 9, 2024 12:34
@wkozyra95 wkozyra95 marked this pull request as ready for review January 9, 2024 12:41
ARG USERNAME=compositor

ENV DEBIAN_FRONTEND=noninteractive
ENV NVIDIA_DRIVER_CAPABILITIES compute,graphics,utility
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be?

Suggested change
ENV NVIDIA_DRIVER_CAPABILITIES compute,graphics,utility
ENV NVIDIA_DRIVER_CAPABILITIES=compute,graphics,utility

Copy link
Member Author

@wkozyra95 wkozyra95 Jan 9, 2024

Choose a reason for hiding this comment

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

this was copy paste from original dockerfile
as far as I remember both syntax are valid, but yes we should be consistent

Copy link
Member

Choose a reason for hiding this comment

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

Idk if slim is self-explanatory. Maybe so use some name indicating that it removes the web renderer

Copy link
Member Author

@wkozyra95 wkozyra95 Jan 9, 2024

Choose a reason for hiding this comment

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

Idk if slim is self-explanatory

You are assuming here that slim image means that it's compositor without web renderer :D

It depends what this file is suppose to mean.

  • If it is intended to be smaller image with minimal set of feature that do not have any significant downsides (e.g. large size, security issues) then this name is self explanatory and consistent with existing naming conventions
  • If it is intended to be full compositor with just web renderer disabled then yes name is not self-explantory

I decided to go with slim because from those 2 approaches it seem to me slightly better, but I don't feel strongly about it.


- uses: hadolint/[email protected]
with:
dockerfile: build-tools/docker/full.Dockerfile

- name: Build image
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't really know. If we're not using too much of the CI time then maybe we should build both versions, but I don't know how to check these limits/the spending

@wkozyra95 wkozyra95 merged commit 901c644 into master Jan 10, 2024
2 checks passed
@wkozyra95 wkozyra95 deleted the @wkozyra95/docker-image-variants branch January 10, 2024 12:28
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.

3 participants