-
-
Notifications
You must be signed in to change notification settings - Fork 58
ignore generated assets folder #751
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
Conversation
* Fix CHECKOV Docker security issues (brisbanesocialchess#554) * Update Dockerfile update the code * Update Dockerfile --------- Co-authored-by: John Bampton <[email protected]>
* Refactor tailwind.css: Change import statement to use URL and reorganize hover effect styles * Refactor role styles in tailwind.css: Consolidate border and color properties for roles * Fix import statement and clean up whitespace in tailwind.css
…ss#580) * Remove duplicate npm-ci hook; refactor local hooks * Update .pre-commit-config.yaml
* Natural language fixes in Markdown files * Update DEVELOPER_HELP.md Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> * Update README.md Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> --------- Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
* pre-commit prettier check JS files JS files were not being checked so there was a difference when running pre-commit and prettier manually via `npm run format`
) * Format base.njk with Prettier and run pre-commit hooks * Optimize PNGs via pre-commit (oxipng) * Unchanged package-lock.json and package.json * Removed Cdn * Apply pre-commit autofixes (EOF newline, oxipng) * Update frontend/_includes/layouts/base.njk --------- Co-authored-by: John Bampton <[email protected]>
* Create .deepsource.toml * Update .deepsource.toml * Update .deepsource.toml Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> * Update .deepsource.toml Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> * Update .deepsource.toml * Update .deepsource.toml * Update .deepsource.toml * Update .deepsource.toml * Update .deepsource.toml * Update .deepsource.toml * Update .deepsource.toml * Update .deepsource.toml * Update .deepsource.toml * Update .deepsource.toml * Update .deepsource.toml Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> * Update .deepsource.toml Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> * Update .deepsource.toml Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> * Update .deepsource.toml * Update .deepsource.toml * Update .deepsource.toml * Update .deepsource.toml Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> * Update .deepsource.toml Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> * Update .deepsource.toml Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> * Update .deepsource.toml Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> * Update .deepsource.toml Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> --------- Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
* Update .deepsource.toml
* Update .deepsource.toml * Update .deepsource.toml Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> --------- Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
* Validate codecov.yml file with curl * Fix up
* Test Docker with GitHub Actions * Update .github/workflows/docker-ci.yml
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
Please resolve the conflicts |
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces several valuable improvements, including adding ignore files for generated assets, setting up CSS linting with Stylelint, and refactoring the Dockerfile for multi-architecture support and better security. My review focuses on a few areas to further enhance these changes: I've identified a bug in the updated README.md
instructions, along with opportunities to simplify the Dockerfile
and clean up some redundant CSS. Overall, this is a solid contribution to improving the project's development workflow and code quality.
README.md
Outdated
docker build -t my-go-precommit . | ||
mkdir docker_data | ||
docker buildx build --platform=linux/arm64 -t ubuntu-pre-commit . | ||
docker run --platform=linux/arm64 -it -v "$PWD":/app -v "$PWD/docker_data/.cache/pre-commit":/root/.cache/pre-commit -v "$PWD/docker_data/.npm":/root/.npm -w /app ubuntu-pre-commit:latest bash |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The volume mount path for the pre-commit and npm caches is incorrect. The Dockerfile now runs as the appuser
with /home/appuser
as its home directory, but this command attempts to mount the cache to /root/...
. This will cause the caching to fail. The path inside the container should be updated to point to the appuser
's home directory.
docker run --platform=linux/arm64 -it -v "$PWD":/app -v "$PWD/docker_data/.cache/pre-commit":/root/.cache/pre-commit -v "$PWD/docker_data/.npm":/root/.npm -w /app ubuntu-pre-commit:latest bash | |
docker run --platform=linux/arm64 -it -v "$PWD":/app -v "$PWD/docker_data/.cache/pre-commit":/home/appuser/.cache/pre-commit -v "$PWD/docker_data/.npm":/home/appuser/.npm -w /app ubuntu-pre-commit:latest bash |
README.md
Outdated
docker run --platform=linux/amd64 --rm -v "$PWD":/app -w /app my-go-precommit | ||
mkdir docker_data | ||
docker buildx build --platform=linux/amd64 -t ubuntu-pre-commit . | ||
docker run --platform=linux/amd64 -it -v "$PWD":/app -v "$PWD/docker_data/.cache/pre-commit":/root/.cache/pre-commit -v "$PWD/docker_data/.npm":/root/.npm -w /app ubuntu-pre-commit:latest bash |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to the arm64
command, the volume mount path for the amd64 architecture is incorrect. It should point to /home/appuser
instead of /root
to align with the non-root user configured in the Dockerfile. This fix is necessary for the caching to work as expected.
docker run --platform=linux/amd64 -it -v "$PWD":/app -v "$PWD/docker_data/.cache/pre-commit":/root/.cache/pre-commit -v "$PWD/docker_data/.npm":/root/.npm -w /app ubuntu-pre-commit:latest bash | |
docker run --platform=linux/amd64 -it -v "$PWD":/app -v "$PWD/docker_data/.cache/pre-commit":/home/appuser/.cache/pre-commit -v "$PWD/docker_data/.npm":/home/appuser/.npm -w /app ubuntu-pre-commit:latest bash |
Dockerfile
Outdated
RUN curl -LO https://mirrors.aliyun.com/golang/go${GO_VERSION}.linux-${TARGETARCH}.tar.gz && \ | ||
tar -C /usr/local -xzf go${GO_VERSION}.linux-${TARGETARCH}.tar.gz && \ | ||
rm go${GO_VERSION}.linux-${TARGETARCH}.tar.gz |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using a specific regional mirror like mirrors.aliyun.com
for downloading Go can be slow or unreliable for developers in other parts of the world. It's better to use the official download URL (https://go.dev/dl/
) to ensure consistent and reliable access for all contributors. This also avoids tying the build process to a third-party mirror that might change or become unavailable.
RUN curl -LO https://go.dev/dl/go${GO_VERSION}.linux-${TARGETARCH}.tar.gz && \
tar -C /usr/local -xzf go${GO_VERSION}.linux-${TARGETARCH}.tar.gz && \
rm go${GO_VERSION}.linux-${TARGETARCH}.tar.gz
background-image: -webkit-gradient(linear, left top, left bottom, from(rgb(0 0 0 / 0%)), to(rgb(0 0 0 / 15%))); | ||
background-image: linear-gradient(to top, rgb(0 0 0 / 0%), rgb(0 0 0 / 15%)); | ||
background-image: linear-gradient(to top, rgb(0 0 0 / 0%), rgb(0 0 0 / 15%)); | ||
background-image: linear-gradient(to top, rgb(0 0 0 / 0%), rgb(0 0 0 / 15%)); | ||
background-image: linear-gradient(to top, rgb(0 0 0 / 0%), rgb(0 0 0 / 15%)); | ||
background-image: linear-gradient(to bottom, rgb(0 0 0 / 0%), rgb(0 0 0 / 15%)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are multiple background-image
declarations here, some of which are duplicates or use outdated syntax. You can simplify this by keeping only the modern, standard linear-gradient
property. The original vendor-prefixed properties were all equivalent to to bottom
, so the to top
versions appear to be a mistake.
background-image: linear-gradient(to bottom, rgb(0 0 0 / 0%), rgb(0 0 0 / 15%));
DEFAULT_BRANCH: main | ||
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} | ||
PYTHON_PYLINT_CONFIG_FILE: .pylintrc | ||
VALIDATE_ALL_CODEBASE: true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please don't change this
this PR add ignore for generated assets folder from vite build