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.

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

Conversation

kishore03109
Copy link
Contributor

@kishore03109 kishore03109 commented Apr 30, 2024

Problem

When there was no records that were found, dns:node throws an error. While we did handle it, there should have been an additional return statement to exit early rather than propagating into an error state.

Solution

If the error is of type ENODATA, return early with some default values instead.

Breaking Changes

  • Yes - this PR contains breaking changes
    • Details ...
  • No - this PR is backwards compatible with ALL of the following feature flags in this doc

Tests

Locally enter these values into support/routes/v2/formsg/index.ts

formsgSiteLaunchRouter
  .digAAAADomainRecords({
    primaryDomainSource: "singaopreoae.sg",
  } as SiteLaunchResult)
  .then(console.log)
  .catch(console.error)

formsgSiteLaunchRouter
  .digCAADomainRecords({
    primaryDomainSource: "singaopreoae.sg",
  } as SiteLaunchResult)
  .then(console.log)
  .catch(console.error)

assert that the console log outputs

Screenshot 2024-04-30 at 9.24.39 AM.png

Deploy Notes

This fix is meant to go into prod line asap

Copy link
Contributor Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @kishore03109 and the rest of your teammates on Graphite Graphite

@kishore03109 kishore03109 marked this pull request as ready for review April 30, 2024 01:34
@kishore03109 kishore03109 requested a review from a team April 30, 2024 01:34
@@ -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

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

@kishore03109 kishore03109 merged commit a840ed0 into develop Apr 30, 2024
19 checks passed
@mergify mergify bot deleted the 04-30-fix_sitelaunch_do_not_throw_for_no_dig_results branch April 30, 2024 01:48
@kishore03109 kishore03109 mentioned this pull request Apr 30, 2024
Comment on lines 257 to 258
logger.error(
`Error when trying to get AAAA records for domain ${launchResult.primaryDomainSource}: ${e}`
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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants