-
-
Notifications
You must be signed in to change notification settings - Fork 105
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/add health check #113 #115
Feature/add health check #113 #115
Conversation
Signed-off-by: Ahmet Turkmen <[email protected]>
Signed-off-by: Ahmet Turkmen <[email protected]>
Signed-off-by: Ahmet Turkmen <[email protected]>
Note; some improvements could be done. |
|
||
var ( | ||
// should be improved | ||
Port = config.SetupConfigDefaults().Server.Port |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think, it is NOT proper way of getting port from configuration. could be improved. (Maybe organization of packages may change )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right, it also bothers my eye. I will also think about it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could be a static variable for test purposes.
Port = config.SetupConfigDefaults().Server.Port | |
Port = "3625" |
Signed-off-by: Ahmet Turkmen <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a great PR. I just have one comment to understand it better. Also I am glad to see test on the project 👍
|
||
var ( | ||
// should be improved | ||
Port = config.SetupConfigDefaults().Server.Port |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right, it also bothers my eye. I will also think about it.
Can you change auth_api.go file name to auth.go. Then I think the conflict will be resolved. |
Signed-off-by: Ahmet Turkmen <[email protected]>
This is great job. Thank you @mrturkmen06 |
* Create CONTRIBUTING.md * Translation (#103) * used const and var for strings * update * update * CONTRIBUTING.md file translated into Turkish. * Update CONTRIBUTING.md * Update CONTRIBUTING.md * Update CONTRIBUTING-TR.md * pre-generated issue template added (#106) Signed-off-by: Ahmet Turkmen <[email protected]> * Removed unnecessary alpine image (#109) * Remove unnecessary alpine image * Fix mkdir command for docker Signed-off-by: Kaan Karakaya <[email protected]> * Fix libc for docker Signed-off-by: Kaan Karakaya <[email protected]> * password package implemented (#112) * password package implemented Signed-off-by: Ahmet Turkmen <[email protected]> * app/enc/password func updated Signed-off-by: Ahmet Turkmen <[email protected]> * rename and delete deprecated todo Signed-off-by: Ahmet Turkmen <[email protected]> * Feature/add health check #113 (#115) * check token earlier Signed-off-by: Ahmet Turkmen <[email protected]> * add /health endpoint Signed-off-by: Ahmet Turkmen <[email protected]> * add workflow to run tests Signed-off-by: Ahmet Turkmen <[email protected]> * return NoToken Err Signed-off-by: Ahmet Turkmen <[email protected]> * remove suffix Signed-off-by: Ahmet Turkmen <[email protected]> * Closes #118 * update health test * workflow db user info Co-authored-by: Recep Alaca <[email protected]> Co-authored-by: Ahmet Türkmen <[email protected]> Co-authored-by: Kaan Karakaya <[email protected]>
Close #113
Summary:
Related test result could be observed from here