Conversation
Summary of ChangesHello @Lokowitz, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request focuses on a significant refactoring of the project's dependency management and build processes. It involves a comprehensive migration to a more modern and potentially more performant YAML processing library, alongside a thorough cleanup of unused packages. Furthermore, the Docker build pipeline has been streamlined for efficiency and clarity, and the Dependabot configuration has been simplified to improve maintainability. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request primarily focuses on migrating from the js-yaml library to the yaml library, which involved updating import statements and replacing yaml.load with yaml.parse and yaml.dump with yaml.stringify across numerous files. Additionally, the Dockerfile has been refactored for improved readability and maintainability by consolidating build steps into npm run scripts and optimizing OCI image label placement. Several unused dependencies were also removed from package.json.
| fs.writeFileSync( | ||
| traefikDynamicConfigPath, | ||
| yaml.dump(traefikConfig, { noRefs: true }), | ||
| yaml.stringify(traefikConfig), |
There was a problem hiding this comment.
The noRefs: true option was removed from yaml.dump when switching to yaml.stringify. If traefikConfig contains objects with circular references or repeated structures, yaml.stringify might produce different YAML output (e.g., duplicating content instead of using references) compared to js-yaml with noRefs: true. This could potentially lead to larger file sizes or functional differences if Traefik's YAML parser behaves differently. Please verify if traefikConfig can have such structures and if the new behavior is acceptable.
|
|
||
| // Keep a copy of the original config for comparison | ||
| const originalConfigYaml = yaml.dump(dynamicConfig, { noRefs: true }); | ||
| const originalConfigYaml = yaml.stringify(dynamicConfig); |
There was a problem hiding this comment.
Similar to the previous comment, the noRefs: true option was removed from yaml.dump. This could lead to functional differences if dynamicConfig contains circular references or repeated structures that were previously handled by js-yaml's noRefs option. Please ensure this change does not negatively impact the generated YAML or Traefik's interpretation.
|
|
||
| // Only write the config if it has changed | ||
| const newConfigYaml = yaml.dump(dynamicConfig, { noRefs: true }); | ||
| const newConfigYaml = yaml.stringify(dynamicConfig); |
There was a problem hiding this comment.
The noRefs: true option was removed from yaml.dump. This could lead to functional differences if dynamicConfig contains circular references or repeated structures that were previously handled by js-yaml's noRefs option. Please ensure this change does not negatively impact the generated YAML or Traefik's interpretation.
|
|
||
| // Keep a copy of the original config for comparison | ||
| const originalConfigYaml = yaml.dump(dynamicConfig, { noRefs: true }); | ||
| const originalConfigYaml = yaml.stringify(dynamicConfig); |
There was a problem hiding this comment.
The noRefs: true option was removed from yaml.dump. This could lead to functional differences if dynamicConfig contains circular references or repeated structures that were previously handled by js-yaml's noRefs option. Please ensure this change does not negatively impact the generated YAML or Traefik's interpretation.
|
|
||
| // Only write the config if it has changed | ||
| const newConfigYaml = yaml.dump(dynamicConfig, { noRefs: true }); | ||
| const newConfigYaml = yaml.stringify(dynamicConfig); |
There was a problem hiding this comment.
The noRefs: true option was removed from yaml.dump. This could lead to functional differences if dynamicConfig contains circular references or repeated structures that were previously handled by js-yaml's noRefs option. Please ensure this change does not negatively impact the generated YAML or Traefik's interpretation.
| RUN if [ "$BUILD" = "oss" ]; then rm -rf server/private; fi && \ | ||
| npm run set:$DATABASE && \ | ||
| npm run set:$BUILD && \ | ||
| npm run db:$DATABASE:generate && \ | ||
| npm run build:$DATABASE && \ | ||
| npm run build:cli |
There was a problem hiding this comment.
The consolidation of multiple RUN commands into a single RUN command utilizing npm run scripts is a significant improvement. This enhances readability and makes the build process more maintainable by centralizing complex logic within package.json.
| # OCI Image Labels - Build Args for dynamic values | ||
| ARG VERSION="dev" | ||
| ARG REVISION="" | ||
| ARG CREATED="" | ||
| ARG LICENSE="AGPL-3.0" | ||
|
|
||
| # Derive title and description based on BUILD type | ||
| ARG IMAGE_TITLE="Pangolin" | ||
| ARG IMAGE_DESCRIPTION="Identity-aware VPN and proxy for remote access to anything, anywhere" |
There was a problem hiding this comment.
Moving the OCI Image Labels and description build arguments to the runner stage is a good optimization. These metadata fields are only relevant for the final image, not during the build process, leading to a cleaner and potentially smaller intermediate builder image.
| COPY --from=builder /app/.next/static ./.next/static | ||
| COPY --from=builder /app/dist ./dist | ||
| COPY --from=builder /app/init ./dist/init | ||
| COPY --from=builder /app/server/migrations ./dist/init |
There was a problem hiding this comment.
The change in the migration copy path from /app/init to /app/server/migrations indicates a restructuring of the build output. Please ensure that the npm run build:$DATABASE script correctly places the migration files in /app/server/migrations for the runner stage to pick them up.
| "email": "email dev --dir server/emails/templates --port 3005", | ||
| "build:cli": "node esbuild.mjs -e cli/index.ts -o dist/cli.mjs", | ||
| "format": "prettier --write ." | ||
| "format": "npx prettier --write ." |
There was a problem hiding this comment.
Changing prettier --write . to npx prettier --write . is a good practice. It ensures that the locally installed prettier package is used, avoiding potential global prettier conflicts or missing executables in different environments.
| "cookie-parser": "1.4.7", | ||
| "cookies": "0.9.1", | ||
| "cors": "2.8.5", | ||
| "crypto-js": "4.2.0", | ||
| "d3": "7.9.0", | ||
| "date-fns": "4.1.0", | ||
| "drizzle-orm": "0.45.1", | ||
| "eslint": "9.39.2", | ||
| "eslint-config-next": "16.1.0", | ||
| "express": "5.2.1", | ||
| "express-rate-limit": "8.2.1", | ||
| "glob": "13.0.0", | ||
| "helmet": "8.1.0", | ||
| "http-errors": "2.0.1", | ||
| "i": "0.3.7", | ||
| "input-otp": "1.4.2", | ||
| "ioredis": "5.9.2", | ||
| "jmespath": "0.16.0", | ||
| "js-yaml": "4.1.1", |
There was a problem hiding this comment.
The removal of cookie, cookies, eslint, eslint-config-next, i, and js-yaml from dependencies is noted. While js-yaml removal is expected due to the switch to the yaml library, please confirm that eslint and eslint-config-next are no longer direct dependencies or have been moved to devDependencies if still used for linting. The format script still uses prettier, which was also removed from devDependencies in DIFF 5. It's generally better to explicitly list tools used in scripts as devDependencies for clarity and to ensure they are installed.
|
Warning This image may contain unchecked and breaking changes. Only use on own risk. 👋 Thanks for your PR! SQLITE Image: Postgresql Image: |
1 participant
Community Contribution License Agreement
By creating this pull request, I grant the project maintainers an unlimited,
perpetual license to use, modify, and redistribute these contributions under any terms they
choose, including both the AGPLv3 and the Fossorial Commercial license terms. I
represent that I have the right to grant this license for all contributed content.
Description
How to test?