-
Notifications
You must be signed in to change notification settings - Fork 7
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 API #58
Feature: Add API #58
Conversation
…ing for other users
DeepCode's analysis on #a44faf found:
Top issues
👉 View analysis in DeepCode’s Dashboard | Configure the bot |
src/handlers/api.ts
Outdated
const lockOwner = await lockRepo.getOwner(lockName, channel, team); | ||
if (!lockOwner) { | ||
console.log("No lock to delete", { lockName }); | ||
res.status(204).end(); |
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.
Would returning a 410 Gone error work here instead of 204?
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.
Nice spot 🧐
410
is an interesting one, I've never used it! It might not be quite accurate all the time because can something be Gone
if it was never there in the first place? Or only Gone
if it was previously locked?
This was one where I wasn't quite sure what to return and opted for a 2XX
/success code over 4XX
because I assumed that clients wouldn't care (or want an error) for when they're trying to delete a lock and there isn't one. But my assumption might be very wrong.
src/handlers/api.ts
Outdated
@@ -0,0 +1,204 @@ | |||
import awsServerlessExpress from "aws-serverless-express"; |
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'm not too familiar with serverless express but it sounds interesting, what does it give that plain serverless does not?
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'm not too familiar with it either 😁
The short answer is that I don't think Express gives you anything particularly special over standard AWS Lambda/Serverless.
aws-serverless-express
is just an easy way to get Express to work with AWS lambda.
It means if you're used to writing apps using Express you can continue doing so. For example by creating and using Express middleware like authorizer
.
The only reason I chose to use Express was because Bolt, the Slack library I'm using in the other handler in this project, also uses Express so I thought I'd use it for consistency.
There's other newer Node web frameworks on the block like Koa and Fastify but I've only really dabbled in AWS Lambda and Express thus far ...
This is looking great, thanks Connor! I think our use for this would be to block deployments to our dev environments when someone is using it. With the API, we should be able to add a check to our CI pipelines to block our dev deployments should it be locked. I'm not sure how much demand we have in our team for such blocking feature at the moment as we haven't had much need to use dev presently, but I would love to see if the rest of the team would like to trial it on one of our repositories. One thing that would be nice to have would be expiring locks. It looks like a lock will exist until they unlock it themselves. An option to set an expiry on a lock may be nice to make sure someone doesn't block a resource longer than needed. |
Fixes #60 |
SonarCloud Quality Gate failed. 0 Bugs No Coverage information |
Closes #43
The API offers the following functionality:
TODO
REAMDE.md