-
Notifications
You must be signed in to change notification settings - Fork 17
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
Add admission service #101
Conversation
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.
Logic is good, some more little JS errors!
Also, could you add some tests for this file (reference changes made in #89 )
@npunati27 One more thing, please don't forget to update docs! |
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.
So far, so good. A few things to consider.
Also, due to recent changes, you'll have to merge in the changes from main, which mainly consists of StatusCode
being added (#106), so your code will have to be updated to use that instead. Also, tests will then use StatusCode.SuccessOK instead of 200 etc.
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.
LGTM! Just make those small changes, and then we should be good to go
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.
So far so good! A few minor changes to be made to make tests & router a little cleaner.
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.
Great job! My only comments left are minor nps & cleaning up some of the stuff from earlier. Code wise, you're golden! By the way, I'm sorry for being pretty strict with these tests but I think it will help preserve the usability of them for years to come.
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.
Looks great! Feel free to merge whenever!
Note: when merging, do a squash merge (from GitHub UI), and edit the description to have only relevant changes. For example, something like "Add GET and PUT endpoints" is good but something like "fix nitpicks" doesn't need to be in the merge description.
admission service, get and put requests.