Skip to content

ci: add GitHub Actions workflow#4

Open
kagurachen28-prog wants to merge 1 commit intoiamtouchskyer:mainfrom
kagurachen28-prog:ci/github-actions-v2
Open

ci: add GitHub Actions workflow#4
kagurachen28-prog wants to merge 1 commit intoiamtouchskyer:mainfrom
kagurachen28-prog:ci/github-actions-v2

Conversation

@kagurachen28-prog
Copy link

Summary

Re-submission of PR #2 (which was approved ✅ but accidentally closed along with PR #1).

What This PR Does

Adds a GitHub Actions CI workflow that:

  • Runs npm test on every push to main and on pull requests
  • Tests against Node.js 18 and 20
  • Uses npm ci for reproducible dependency installation
  • Provides test environment variables (non-sensitive CI placeholders)

Changes from PR #2

  • Identical workflow, same approach that was approved
  • Added ADMIN_USERNAME, ADMIN_PASSWORD, ALLOWED_ORIGINS env vars for tests that may need them

File

  • .github/workflows/ci.ymlnew

Note

No lint step is included since the project doesn't have an ESLint/Prettier configuration yet. That could be a good follow-up improvement.

- Run tests on push to main and on pull requests
- Test against Node.js 18 and 20
- Use npm ci for reproducible installs
- Provide test environment variables (non-sensitive placeholders)

Re-submission of PR iamtouchskyer#2 (which was approved but closed).
Copy link
Owner

@iamtouchskyer iamtouchskyer left a comment

Choose a reason for hiding this comment

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

✅ Approved

Clean CI setup:

  1. Matrix testing - Tests on Node 18 and 20
  2. Proper env handling - Uses test-only secrets, not real credentials
  3. npm ci - Good for reproducible builds

Minor suggestion (non-blocking)

Consider adding cache: 'npm' alongside node-version setup for faster CI runs. But this is already well-configured.

Ready to merge!

Copy link
Owner

@iamtouchskyer iamtouchskyer left a comment

Choose a reason for hiding this comment

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

✅ LGTM

Clean and straightforward CI workflow.

Good practices:

  • Tests against Node 18 and 20 for compatibility
  • Uses npm ci for reproducible builds
  • Non-sensitive placeholder env vars for CI
  • Triggers on both push to main and PRs

Future improvements to consider:

  • Add lint step when ESLint is configured
  • Consider caching node_modules for faster runs (though cache: npm helps)

Ready to merge! 🚀

Copy link
Owner

@iamtouchskyer iamtouchskyer left a comment

Choose a reason for hiding this comment

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

Review Summary

This PR adds a standard CI workflow for the project.

✅ Good Practices

  • Tests on both Node 18 and 20 (matrix strategy)
  • Uses npm ci for reproducible builds
  • Sets appropriate test environment variables
  • Uses latest action versions (v4)

💡 Suggestion (optional)

Consider adding a lint job to the workflow:

- name: Run lint
  run: npm run lint

LGTM — Standard CI setup for Node.js projects.

Copy link
Owner

@iamtouchskyer iamtouchskyer left a comment

Choose a reason for hiding this comment

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

LGTM! Good CI setup with matrix testing on Node 18/20. Environment variables for tests are properly configured.

Copy link
Owner

@iamtouchskyer iamtouchskyer left a comment

Choose a reason for hiding this comment

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

✅ LGTM! Solid CI setup with Node.js matrix testing and proper environment variables. Note: If tests require database connectivity, you may want to add a service container for SQLite/testing DB.

Copy link
Owner

@iamtouchskyer iamtouchskyer left a comment

Choose a reason for hiding this comment

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

LGTM! 🚀 Solid CI setup with Node 18/20 matrix testing and proper test environment configuration. Good practice using npm ci and latest Action versions.

Copy link
Owner

@iamtouchskyer iamtouchskyer left a comment

Choose a reason for hiding this comment

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

LGTM! Well-configured GitHub Actions workflow:

  • Matrix strategy testing Node 18 & 20
  • Proper test environment variables
  • Uses latest actions (checkout@v4, setup-node@v4)
  • Includes necessary secrets for JWT/session testing
  • Runs on push to main and PRs

This will catch issues early and ensure compatibility across Node versions.

Copy link
Owner

@iamtouchskyer iamtouchskyer left a comment

Choose a reason for hiding this comment

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

LGTM! Solid CI workflow that tests on Node 18/20, uses modern GitHub Actions versions, and includes appropriate test environment variables. Good addition for maintaining code quality.

Copy link
Owner

@iamtouchskyer iamtouchskyer left a comment

Choose a reason for hiding this comment

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

LGTM! CI工作流配置合理:支持Node.js 18和20两个版本,包含了所有必要的测试环境变量。这将帮助确保代码质量和兼容性。

Copy link
Owner

@iamtouchskyer iamtouchskyer left a comment

Choose a reason for hiding this comment

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

LGTM! Well-configured CI workflow with Node.js matrix testing (18, 20), proper test environment variables, and latest GitHub Actions versions. Good security practice using test-only secrets.

@kagurachen28-prog
Copy link
Author

Hi @iamtouchskyer! 👋 Same here — CI workflow is ready to go. Let me know if you'd like any changes!

Copy link
Owner

@iamtouchskyer iamtouchskyer left a comment

Choose a reason for hiding this comment

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

Well-configured GitHub Actions workflow! Tests on Node 18 & 20, uses modern action versions, and includes proper CI environment variables. The npm ci usage is good for reproducible builds. This will catch issues early. LGTM! 🚀

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.

2 participants