Skip to content

Conversation

@NalinDalal
Copy link

πŸ“ Description

introduction of ci/cd pipeline so that manual checks such as code compilation can be skipped by automating them
Fixes #16


πŸ”§ Type of Change

  • Bug fix πŸ›
  • New feature ✨
  • Refactor ♻️
  • Documentation update πŸ“š
  • Test update πŸ§ͺ
  • Chore / Build system βš™οΈ

πŸš€ Changes Made

  • Added workflows for ci/cd

βœ… Checklist

  • I have tested my code locally
  • I have added necessary unit/integration tests
  • I have updated related documentation (README, Wiki, etc.)
  • My changes do not introduce new warnings or errors
  • I have assigned reviewers and linked relevant issues

server/Makefile Outdated
@@ -0,0 +1,28 @@
APP_NAME := myapp
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this is a Go backend, consider removing the Makefile for now. Without a deployment target decided, it’s simpler to build directly with Go. Once hosting is finalized, we can add a CD script for build and deployment.

Copy link
Contributor

Choose a reason for hiding this comment

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

FYI A Makefile is generally more useful when:

  1. You need to build for multiple platforms or architectures.
  2. You want to orchestrate multiple build/test/lint steps.
  3. You're managing CI/CD pipelines with reusable scripts.
  4. You're dealing with native C/C++ dependencies or multi-language projects.

Comment on lines 23 to 25
if _, err := w.Write([]byte("ok")); err != nil {
log.Printf("failed to write response: %v", err)
}
Copy link
Contributor

@satyajitnayk satyajitnayk May 28, 2025

Choose a reason for hiding this comment

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

Remove the error check on w.Write β€” it’s unlikely to fail for a simple "ok" and only adds clutter.

Linters may suggest this error check, but here it’s unnecessary since the write is trivial and failure unlikely.

Comment on lines 60 to 64
defer func() {
if cerr := file.Close(); cerr != nil {
log.Printf("warning: failed to close file: %v", cerr)
}
}()
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
defer func() {
if cerr := file.Close(); cerr != nil {
log.Printf("warning: failed to close file: %v", cerr)
}
}()
defer file.Close()

Comment on lines 51 to 54
rootDir, err := os.Getwd()
if err != nil {
return "", err
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
rootDir, err := os.Getwd()
if err != nil {
return "", err
}
rootDir, err := os.Getwd()

@@ -0,0 +1,58 @@
name: CI/CD Pipeline
Copy link
Contributor

Choose a reason for hiding this comment

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

will review later ⏳

Comment on lines 32 to 41
- name: Build
run: go build ./...
working-directory: server

- name: Run Make Test
run: make test
- name: Test
run: go test ./...
working-directory: server

- name: Run Make Lint
run: make lint
- name: Lint
run: golangci-lint run ./...
Copy link
Contributor

Choose a reason for hiding this comment

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

The logical order should be: lint β†’ test β†’ build.

Why:

  1. Lint first: Catch obvious syntax/style issues early before running expensive operations.
  2. Test next: Ensure correctness before compiling for deployment.
  3. Build last: Only build if the code is clean and verified.

@satyajitnayk
Copy link
Contributor

Note for Approver:
Please hold off on approving CI/CD setup until test cases are added in a separate PR β€” this ensures we verify code correctness before automating deployments.

@NalinDalal
Copy link
Author

sure, so i will close the pull request for now, and make the required changes, will later open when test cases are added

@NalinDalal NalinDalal closed this May 29, 2025
@satyajitnayk
Copy link
Contributor

sure, so i will close the pull request for now, and make the required changes, will later open when test cases are added

You can mark it as draft or do not close at all. I have added comment for approver

@NalinDalal NalinDalal reopened this May 29, 2025
@satyajitnayk
Copy link
Contributor

@NalinDalal Could you add some test cases for important logics?
Let me know if you need any input

@peachjelly13
Copy link
Member

@NalinDalal please look at the issue addressing adding test cases

@NalinDalal
Copy link
Author

busy with examsπŸ₯Ά will do surely @peachjelly13 @satyajitnayk

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.

setup CI/CD pipeline

3 participants