Feedback #4

coolaj86 · 2020-08-19T06:35:24Z

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...

The text was updated successfully, but these errors were encountered:

rtomlinson-latacora · 2020-08-26T22:00:23Z

Hey AJ,

Thanks for taking a look at the code. I'll make some improvement using your stylistic and syntactic recommendations.

The reasoning I had for implementing the lock was the the GCP DNS API only allowed me to make requests for changes, but does not actually block until the change has been made. This was causing issues regarding certs with wildcards. It seemed that for some of the DNS provider APIs that a change request would wait until the change was completed before returning. I didn't see anything in the Greenlock code base that addressed this, but I most definitely could have missed it.
Are you suggesting that maybe instead of implementing the lock that I could query the existing change requests and see if the request had been made? I feel that that approach is a little more brittle, but I could look into it.

Thanks again

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feedback #4

Feedback #4

Feedback #4

Feedback #4

Comments

Cruft in Readme

De-nest ifs

Why lock?

De-nest promises

code style