search

okuda-seminar / Twitter-Clone

0 stars 0 forks source link

[Infra] Add ingress for notification service #347

Closed Rwatana closed 4 months ago

Rwatana commented 4 months ago

Issue Number

257

Implementation Summary

I implemented Ingress. Specifically, the notification backend service doesn't exist, resulting in a 502 error. However, I believe this is acceptable for now.

Scope of Impact

This file will be executed during the environment setup.

Particular points to check

Please review and let me know if you have any concerns, comments, or questions.

Test

 Twitter-Clone git:(feature/#13_add_ingress_for_notification_service) k get ingress
NAME          CLASS   HOSTS   ADDRESS     PORTS   AGE
ingress-srv   nginx   *       localhost   80      27m

 Twitter-Clone git:(feature/#13_add_ingress_for_notification_service) curl localhost:80/api/notifications 
<html>
<head><title>502 Bad Gateway</title></head>
<body>
<center><h1>502 Bad Gateway</h1></center>
<hr><center>nginx</center>
</body>
</html>

Schedule

7/6

Rwatana commented 4 months ago

@nayuta-ai Please check my code.

nayuta-ai commented 4 months ago

@Rwatana Please pass the CI. It caused that your commit message doesn't end with a period. Also, why is the 502 Error occurring?

ryuji0123 commented 4 months ago

Probably notifications service supports only POST method right now.

Ref

@ryuju0911 Please correct me if I'm wrong.

nayuta-ai commented 4 months ago

@Rwatana Please show the example of 200 responses using notifications endpoints. Also, you should create a new Issue for the open status.

It seems like the necessary parameters to send a POST are written here! https://github.com/okuda-seminar/Twitter-Clone/blob/main/go/notifications/gen/http/openapi3.yaml#L15-L23

ryuju0911 commented 4 months ago

As @ryuji0123 and @nayuta-ai mentioned, the Notifications service currently only accepts POST requests. You have to send the request with text and tweet_id.

Rwatana commented 4 months ago

@nayuta-ai

Summary

I modified the code and confirmed that it works properly. Please review my code.

Test

 base git:(feature/#13_add_ingress_for_notification_service) ✗ k get pod                                  
NAME                                      READY   STATUS        RESTARTS   AGE
notifications-app-598bbb65-xwdj5          1/1     Running       0          14s
notifications-app-b894dfc74-m9lxs         1/1     Terminating   0          41s
notifications-postgres-57d964699c-mp2xx   1/1     Running       0          45h
users-app-54985f8dd7-rmnck                1/1     Running       0          22h
users-postgres-59488d5c67-p8hf9           1/1     Running       0          22h
➜  base git:(feature/#13_add_ingress_for_notification_service) ✗ curl -X POST localhost:80/api/notifications -d '{"text":"test", "tweet_id":"1234567"}'

➜  base git:(feature/#13_add_ingress_for_notification_service) ✗ k logs notifications-app-598bbb65-xwdj5                   
[notificationsapi] 12:45:22 HTTP "CreateTweetNotification" mounted on POST /api/notifications
[notificationsapi] 12:45:22 HTTP "./gen/http/openapi.json" mounted on GET /swagger.json
[notificationsapi] 12:45:22 HTTP server listening on "0.0.0.0:80"
[notificationsapi] 12:45:36 id=7cdzcHPZ req=POST /api/notifications from=192.168.65.4
[notificationsapi] 12:45:36 notifications.CreateTweetNotification
[notificationsapi] 12:45:36 id=7cdzcHPZ status=200 bytes=0 time=1.545875ms
➜  base git:(feature/#13_add_ingress_for_notification_service) ✗

Questions

Initially, the container crashed because it stopped after finishing the task and restarted multiple times.

If I add the following line to the notifications-app-depl.yaml file, the container works correctly. However, I believe this command should be written in the Dockerfile instead. If I commit with the change below, the curl -X POST localhost:80/api/notifications -d '{"text":"test", "tweet_id":"1234567"}' command works properly. However, I think this commit should be done in a separate PR. How should I proceed?

          command: ["/bin/sh", "-c", "cd cmd/notifications && go build -o notifications . && ./notifications"]
ryuji0123 commented 4 months ago

I'm glad to see this useful error message. Infra things are apparently important to help developers :)

Rwatana commented 4 months ago

@ryuji0123 I've gotten confused about how to proceed with Git. Is it okay if I ask you for help tomorrow?

nayuta-ai commented 4 months ago

@Rwatana Please see the error message in Validate for PR and Commits and fix that. https://github.com/okuda-seminar/Twitter-Clone/actions/runs/9808800464/job/27085309150?pr=347

nayuta-ai commented 4 months ago

Replay for Questions

OK! Please create a new issue and add TODO comments in the corresponding code.

@ryuju0911 By the way, is it correct that the notification service doesn't send any response?