Skip to content
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.

Sign up for GitHub

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

fix(sitelaunch): do not throw for no dig results #1355

Merged
merged 1 commit into from
Apr 30, 2024
Merged

fix(sitelaunch): do not throw for no dig results #1355

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions support/routes/v2/formsg/formsgSiteLaunch.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -252,6 +252,7 @@ export class FormsgSiteLaunchRouter {
logger.info(
`Domain ${launchResult.primaryDomainSource} does not have any AAAA records.`
)
return [] // no AAAA records found
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

without this return, this ended up throwing an error instead

}
logger.error(
`Error when trying to get AAAA records for domain ${launchResult.primaryDomainSource}: ${e}`
Comment on lines 257 to 258
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/nit since you're editing this file, you could take the opportunity to clean up the neighbouring log statements. The existing statements does not print the error (since errors are not serializable).

Suggested change
logger.error(
`Error when trying to get AAAA records for domain ${launchResult.primaryDomainSource}: ${e}`
logger.error({
message: 'Error when trying to get AAAA records for domain',
error: e,
meta: {
domain: launchResult.primaryDomainSource
}
})

Same can be done for the other error() and info() logs seen as context code in the PR.

But yeah, so main message with enahnced logging:

  • report errors as-is, let the logger figure out how to export it
  • log parameters as fields rather than interpolated content in the message

Copy link
Contributor Author

@kishore03109 kishore03109 Apr 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since merged will create a separate pr #1358 ya

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes Sir 👍 . Thank you 🙏 .

Expand Down Expand Up @@ -315,6 +316,12 @@ export class FormsgSiteLaunchRouter {
logger.info(
`Domain ${launchResult.primaryDomainSource} does not have any CAA records.`
)

// if no CAA records, no need to add Amazon CAA and letsencrypt.org CAA
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

without this return, this ended up throwing an error instead

return {
addAWSACMCertCAA: false,
addLetsEncryptCAA: false,
}
}
logger.error(
`Error when trying to get CAA records for domain ${launchResult.primaryDomainSource}: ${e}`
Expand Down
Loading