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

Feature: Added more documentation and clean code. #20

Merged
merged 11 commits into from
Oct 22, 2024

Conversation

werniq
Copy link
Contributor

@werniq werniq commented Oct 21, 2024

In this Pull Request, I have added a lot of documentation to functions and structs in the project, so that users can more easily understand what is going on.

Additionally, I made these additional changes:

  • Renamed variables and funcs
  • Reformatted Dockerfile
  • Added more descriptive logging when error occurs + handled more errors.
  • Implemented commands in Makefile to build/push docker image.
  • Added taskfile (which In my opinion is more easy to read than Makefile, want the owner to verify it and if needed remove Make)
  • Added ability for user to specify some key variables using flags, since some users may want to specify another port/network interface/etc.

I also would like to open Pull Request which integrates K8S to the project, but it contains updates from this PR. To not duplicate changes, I will do it after we resolve this PR.

Comment on lines +23 to 27
// question: Why do we need context here? It is not used in collector.Run, except of ctx.Done, but since it is not
// context.WithTimeout (as example) it can not be closed in any way.
// Same in c.serializer.SerializePackets(ctx, packetsChan), it can not be closed there as well.
// Why not just to remove it?
ctx := context.Background()
Copy link
Owner

Choose a reason for hiding this comment

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

Tbh I don't remember why I passed it there, you can remove it. Or, if you want to, you can implement some graceful shutdown on the application close for the sake of art 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I will. Probably by capturing signals. I guess I will do another PR with graceful shutdown.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It should be similar to this:

signal.Notify(quit, syscall.SIGINT, syscall.SIGTERM)

go func() {
        if err := srv.ListenAndServe(); err != nil && err != http.ErrServerClosed {
            log.Fatalf("Server failed: %s\n", err)
        }
    }()
log.Println("Server started on :8080")

<-quit
log.Println("Shutdown signal received, shutting down gracefully...")

ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second)
defer cancel()

if err := srv.Shutdown(ctx); err != nil {
        log.Fatalf("Server forced to shutdown: %s\n", err)
}

Copy link
Owner

@thegodenage thegodenage left a comment

Choose a reason for hiding this comment

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

I love what you did here, for real man, great job. Just give me an answer if you will implement the graceful shutdown or not, and I will merge it 😄

@thegodenage thegodenage merged commit 5a31d1a into thegodenage:main Oct 22, 2024
1 check failed
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