GWI Platform project #20

tkrinas · 2024-07-13T07:42:40Z

Implement core server functionality
Add asset management (Chart, Insight, Audience)
Include user and favorites handling
Set up basic authentication middleware
Implement logging with daily rotation
Add integration tests

wiz-gwi · 2024-07-13T07:42:53Z

Wiz Scan Summary


IaC Misconfigurations
Vulnerabilities
Sensitive Data
Total
Secrets

- Implement core server functionality - Add asset management (Chart, Insight, Audience) - Include user and favorites handling - Set up basic authentication middleware - Implement logging with daily rotation - Add integration tests

teliaz

Thank you for submitting the test @tkrinas! Can you answer the following alongside my comments?

Are you following a specific design pattern on you submission?
How do you value code comments and when do you think that they are necessary?

thanks for the effort you put in this assignment!

teliaz · 2024-07-17T09:48:56Z

app/assets/queries.go

@@ -0,0 +1,80 @@
+package assets
+
+const (


Can you explain the reasoning for creating this file? How would this scale in a larger project?

Can you explain the reasoning for creating this file? How would this scale in a larger project?

Keeping all the queries of a package in separate file keeps the code more readable, also and more important some queries can be reusable from various function and thats why I separated them from the code

teliaz · 2024-07-17T10:30:26Z

app/server/app.go

+	// Create a new ServeMux
+	mux := http.NewServeMux()
+
+	// Set up routes


Have you considered approaching this in a more RESTful manner, such as using method-based routing? Since you're not using an external framework, you might find go 1.22 helpful.

to be honest not really since this was working I dint had any strong reason to change approach

teliaz · 2024-07-17T10:38:38Z

app/database/database.go

+}
+
+// CloseDB closes the database connection
+func CloseDB() {


I was wondering if this function is currently being used anywhere in the codebase. Could you please clarify its usage?

It should be handling the end of the DB connection but I did not used it

teliaz · 2024-07-17T10:40:11Z

app/go.mod

@@ -0,0 +1,17 @@
+module gwi-platform
+
+go 1.20


I noticed that the project is currently using Go 1.20. Is there a specific reason for not opting for the latest version of Go? It might be beneficial to use the latest version for improved features.

again here not a specif reason, since it was working I an POC app I was working before I keep it the same

teliaz · 2024-07-17T10:41:43Z

app/handlers/asset_handlers.go

+		return
+	}
+
+	ctx, cancel := context.WithTimeout(r.Context(), 10*time.Second)


To improve code readability, would it be possible to replace the hardcoded value 10*time.Second with a named constant or configuration variable? This would help avoid the use of magic numbers.

yes this should be a config, so we can set easy without any change in the code, eg this could be set per env based if it is a test/stage/prod deployment

teliaz · 2024-07-17T10:46:33Z

app/handlers/favorite_handlers.go

+
+	db := database.GetDB()
+
+	tx, err := db.BeginTx(ctx, nil)


It's great to see that you've added support for transactional updates in this function. Could you please explain the benefits of using this approach?

typical "all or nothing" insertion, making the request ATOMIC, if something fails everything fails, so we can decide what to do without the need to think what has failed or not, eg the client would have a retry policy and try to resend the exact same request.

teliaz · 2024-07-17T10:50:01Z

app/Dockerfile

@@ -0,0 +1,16 @@
+FROM golang:1.20.5-alpine


Thank you for providing a containerized version here. To reduce the image size, could you consider using a multi-stage build? Are there any other techniques you might consider for minimizing the Docker image size?

we could either have the app to build before we build the image and copy only the binary, eg if we had this set up in a pipeline build, or we can have a builder image to generate the binary and the copy to the runtime image.

teliaz · 2024-07-17T10:57:41Z

docker-compose.yml

+    healthcheck:
+      test: ["CMD-SHELL", "curl -f http://localhost:8080/ping || exit 1"]


Excellent addition here!

teliaz · 2024-07-17T11:03:51Z

app/handlers/favorite_handlers.go

+	resultChan := make(chan []models.FavoriteAssetDetailed, len(shards))
+	var wg sync.WaitGroup
+
+	for _, shard := range shards {


Nice feature @tkrinas, thank you for implementing this! Could you provide some more context on your approach? Specifically, how do you think this approach would scale with a larger number of shards or increased user activity?

I wanted to shard my data so I can serve them more efficient. Also with this approach I wanted to showcase the usage of concurrency, this could be also useful in case that we wanted to do more heavy computations in our data.

The other side of shards are organizing our data to structured parts that we have in control the size of if to avoid network bottlenecks, client processing time, imagine a client that need to receive a very very big json and after needs to render it, this can cause delays and bad customer experience, while having to handle less data we can start showing something until we have load the full thing

Also we can build further on that (especially if the data are not changing so frequently) and cache them at another layer, eg a varnish cache and serve them from there without adding extra load to our backend

another thing that we can do to improve the cache is find a way to have more "stable shards" that would be heavily cached and direct new changes in different shards that will hold the latest additions.
Said that we could have an off line mechanism to reorganize the shards for better performance

tkrinas · 2024-07-17T13:13:09Z

Thank you for submitting the test @tkrinas! Can you answer the following alongside my comments?

Are you following a specific design pattern on you submission?

How do you value code comments and when do you think that they are necessary?

thanks for the effort you put in this assignment!

Hi @teliaz this my crash 3-4 weeks approach to go, I tried to do something close to MVC (Model-View-Controller) based on what I have seen in some tutorials and based by experience from .net a log time go.

Also about comments having a second look I could have added some more, but at least in theory the code should be self explained and we need to leaning on that approach. imagine to have to answer a support call at night and you need to understand whats is went wrong

Initial commit: Add GWI Platform project

fd1e420

- Implement core server functionality - Add asset management (Chart, Insight, Audience) - Include user and favorites handling - Set up basic authentication middleware - Implement logging with daily rotation - Add integration tests

tkrinas force-pushed the feature/develop branch from d0a0874 to fd1e420 Compare July 13, 2024 07:51

teliaz reviewed Jul 17, 2024

View reviewed changes

tkrinas changed the title ~~Initial commit: Add GWI Platform project~~ GWI Platform project Jul 18, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GWI Platform project #20

GWI Platform project #20

tkrinas commented Jul 13, 2024

wiz-gwi bot commented Jul 13, 2024

teliaz left a comment

teliaz Jul 17, 2024

tkrinas Jul 17, 2024

teliaz Jul 17, 2024

tkrinas Jul 17, 2024

teliaz Jul 17, 2024

tkrinas Jul 17, 2024

teliaz Jul 17, 2024

tkrinas Jul 17, 2024

teliaz Jul 17, 2024

tkrinas Jul 17, 2024

teliaz Jul 17, 2024

tkrinas Jul 17, 2024 •

edited

Loading

teliaz Jul 17, 2024

tkrinas Jul 17, 2024

teliaz Jul 17, 2024

teliaz Jul 17, 2024

tkrinas Jul 17, 2024 •

edited

Loading

tkrinas commented Jul 17, 2024

		healthcheck:
		test: ["CMD-SHELL", "curl -f http://localhost:8080/ping \|\| exit 1"]

GWI Platform project #20

Are you sure you want to change the base?

GWI Platform project #20

Conversation

tkrinas commented Jul 13, 2024

wiz-gwi bot commented Jul 13, 2024

Wiz Scan Summary

teliaz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tkrinas Jul 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tkrinas Jul 17, 2024 • edited Loading

Choose a reason for hiding this comment

tkrinas commented Jul 17, 2024

tkrinas Jul 17, 2024 •

edited

Loading

tkrinas Jul 17, 2024 •

edited

Loading