latacora / acme-dns-01-gcp Goto Github PK

First: Looks like you did a good job on this. 👍

I still haven't gotten around to figuring out how to do the google authentication with normal OIDC + JWT. It's fallen off my priority list, maybe forever.

I'm not sure what kind of feedback you wanted, but here are some of my thoughts:

Cruft in Readme

Looks like you copied over some of the example text from the test suite, I'd say remove that so that your work shines through and isn't bogged down with example and test-related text. Also, since I didn't build this you probably don't want to leave my company plug at the bottom 😉

De-nest ifs

I'd recommend trying to de-nest things a little bit.

Where you have stuff like this:

if (conditionA) {
  // do 2 lines of stuff
} else {
  // do 50 lines of stuff
}

try to convert that into:

if (conditionA) {
  // do 2 lines of stuff
  return
}

// do 50 lines of stuff

Or perhaps move the 50 lines of stuff into its own function.

Why lock?

I don't quite understand the purpose of acquireLock. Why would you need to lock a value for a DNS record?

If there are race conditions for your application, I'd recommend that you handle them there. Greenlock already ensures that only one ACME request happens at a time for a given domain, and in general, that sort of check shouldn't live down in the DNS-01 module unless it's specific to some quirk of the specific dns provider.

De-nest promises

It looks like you're using await without really getting the benefit of it - just trading "promise chain hell" for "try catch hell".

When you have an await you don't need to try/catch because you can just do something like this:

var thingy = await doStuff().catch(function (err) {
  return null;
})
if (!thingy) {
  return;
}

code style

lockValue === void(0)

I'd do lockValue === typeof 'undefined' because it's more clear. void() is a very niche feature in javascript. I don't think I've seen it used by more than 3 people (including you).

console.log(x)

You could wrap these to not print all of the time:

var DEBUG = process.env.DEBUG;
if (DEBUG) {
  console.log(DEBUG)
}

I forget to do that often...