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

[SYCL][Devops] Fix DockerFile linting issues discovered by trivy #16411

Merged
merged 15 commits into from
Dec 19, 2024

Conversation

AlexeySachkov
Copy link
Contributor

This is a re-submit of #16290 with fixes from #16324 and some more extra changes.

Issues addressed:

Issues remaining:

I didn't add HEALTHCHECK command to our containers, because I don't know if that makes sense and which command to launch. I.e. our containers they only provide some pre-installed tools, but they don't launch any services which we could check.

User creation was outlined into a separate helper script. Our containers only come with sycl_ci user now which requires a password to use sudo. However, it is still possible to get the original sycl user for those who uses that container locally and needs sudo access.

@AlexeySachkov
Copy link
Contributor Author

Note for reviewers: whilst I'm troubleshooting issues with container builds, I'm cancelling all other affected workflows. Once this patch is ready for review and I receive some feedback, I will do a full CI run with all checks to make sure that my workflow changes don't cause any issues.

Copy link
Contributor

@sarnex sarnex left a comment

Choose a reason for hiding this comment

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

lgtm, all feedback addressed from previous pr

Copy link
Contributor

@sarnex sarnex left a comment

Choose a reason for hiding this comment

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

sorry, just noticed some typos in the doc

sycl/doc/developer/DockerBKMs.md Outdated Show resolved Hide resolved
sycl/doc/developer/DockerBKMs.md Outdated Show resolved Hide resolved
sycl/doc/developer/DockerBKMs.md Outdated Show resolved Hide resolved
sycl/doc/developer/DockerBKMs.md Outdated Show resolved Hide resolved
@sarnex
Copy link
Contributor

sarnex commented Dec 19, 2024

@bader @steffenlarsen Doc overlords, can someone take a look? Thanks!

Need to make some Docker changes soon, so having this PR in will make merge simpler.

@sarnex sarnex merged commit 611d6d2 into sycl Dec 19, 2024
33 checks passed
@AlexeySachkov AlexeySachkov deleted the private/asachkov/apply-docker-best-practices branch December 20, 2024 10:08
AlexeySachkov added a commit that referenced this pull request Dec 20, 2024
Apparently, I overlooked part of the documentation saying that docker
secrets are only available for containers running as a service which
isn't the case for our build/test CI pipeline.

Therefore, I'm partially reverting #16411 so that our containers once
again use `sycl` user which has password-less `sudo` access.

This PR also reverts #16436 to re-enable GPU reset.
AlexeySachkov added a commit that referenced this pull request Dec 20, 2024
Apparently, I overlooked part of the documentation saying that docker
secrets are only available for containers running as a service which
isn't the case for our build/test CI pipeline.

Therefore, I'm partially reverting #16411 so that our containers once
again use `sycl` user which has password-less `sudo` access.

This PR also reverts #16436 to re-enable GPU reset.
sarnex pushed a commit that referenced this pull request Dec 20, 2024
Apparently, I overlooked part of the documentation saying that docker
secrets are only available for containers running as a service which
isn't the case for our build/test CI pipeline.

Therefore, I'm partially reverting #16411 so that our containers once
again use `sycl` user which has password-less `sudo` access.

This PR also reverts #16436 to re-enable GPU reset.
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