Automate reminder mails #3
Conversation
|
@djfarrelly it's ready-ish to roll out. This is pretty rough. I've sent what keysinfo.json looks like to you separately. Right now I'm filling in the details. 😁 |
There was a problem hiding this comment.
Hey @kiriappeee, overall, this looks great!! Nice work 😄 I decided to add some js-syntax/style comments in this PR since you're starting to pick up some more JS in addition to your heavy Python usage. I don't mean to nit-pick, just provide some js tips 😃
Functionality wise, this looks good, but it would be great to have an example keysinfo.json file in the repo.
index.js
Outdated
| const { Users: users} = await iam.listUsers({}).promise() | ||
| stopChecking = false |
There was a problem hiding this comment.
It would be good to use a let stopChecking = false to declare this variable
There was a problem hiding this comment.
On second review, is this variable used anywhere? Can we remove it?
index.js
Outdated
| const getKeyDetails = async ({ | ||
| keyNames | ||
| }) => { | ||
| const keyInfoFileContents = await readFileAsync(join(__dirname, './keys.info')) |
There was a problem hiding this comment.
Can we perhaps add a keys.info.example file to this repo and check it in so future devs can reference that?
There was a problem hiding this comment.
Is this file keys.info or keysinfo.json (see .gitignore)? I think if it's a json file the file should have a .json extension 😄
index.js
Outdated
| ) { | ||
| console.log(`Found Expired Key: ${UserName} - ${key.AccessKeyId} - ${daysOld} days old`) | ||
| expiredKeys.push({ | ||
| user: UserName, | ||
| keyId: key.AccessKeyId, | ||
| daysOld, | ||
| }) | ||
| if (daysOld == daysError || (daysOld-daysError)%12== 0) { |
There was a problem hiding this comment.
In js world it's always preferable to use === instead of == for equality as == can be true in certain cases triple-equals isn't. You may already know this, but if, not just sharing a js tip as I know you're stronger in Python 😉
Could be cool to extract this as a constant like REMINDER_EMAIL_PERIOD or REMDER_EMAIL_DELAY. It could be a top level const
There was a problem hiding this comment.
Re-reading this logic, both sides of the conditional will be truthy if daysOld === daysError. Maybe this could be simplified like...
const daysPastExpiration = daysOld - daysError
if (daysPastExpiration % REMINDER_EMAIL_PERIOD === 0) {
index.js
Outdated
|
|
||
| expiredKeys.forEach((expiredKey) => { | ||
|
|
||
| var compiledExpiredEmail = compile(expiredKeyMailTemplate.toString()) |
There was a problem hiding this comment.
const and let are preferable in javascript to var with modern js. In these cases, I would use const 😄
More background: https://medium.com/javascript-scene/javascript-es6-var-let-or-const-ba58b8dcde75
index.js
Outdated
| expiredKeys | ||
| }) => { | ||
| console.log(expiredKeys) | ||
| expiredKeyMailTemplate = await readFileAsync(join(__dirname, './reminderemail.html')) |
index.js
Outdated
| keyToRotate: expiredKey | ||
| }) | ||
| var msg = { | ||
| to: keyListInfo[expiredKey].recipients.map(recipient => {return recipient.email}), |
There was a problem hiding this comment.
js tip: this could omit the {} and return: recipients.map(recipient => recipient.email)
|
thanks so much for the comments @djfarrelly . Still very new to the Javascript world 😁 |
|
Hey @djfarrelly . Just pushed the changes for this. It now matches the eslint/airbnb spec. I also learnt a ton more about asynchronous programming. Pretty neat. The whole job runs a lot faster now. |
|
Deployed and testing with tomorrow's job |
No description provided.