Skip to content

Conversation

durran
Copy link
Member

@durran durran commented Feb 3, 2025

Description

Fixes the AWS Lambda CI failures.

What is changing?

Removes app insights in the lambda function as we don't use it.

Is there new documentation needed for these changes?

None

What is the motivation for this change?

Double check the following

  • Ran npm run check:lint script
  • Self-review completed using the steps outlined here
  • PR title follows the correct format: type(NODE-xxxx)[!]: description
    • Example: feat(NODE-1234)!: rewriting everything in coffeescript
  • Changes are covered by tests
  • New TODOs have a related JIRA ticket

@durran durran marked this pull request as ready for review February 3, 2025 15:55
@durran durran requested a review from a team as a code owner February 3, 2025 15:55
@baileympearson baileympearson self-assigned this Feb 3, 2025
@baileympearson baileympearson added the Primary Review In Review with primary reviewer, not yet ready for team's eyes label Feb 3, 2025
Copy link
Contributor

@baileympearson baileympearson left a comment

Choose a reason for hiding this comment

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

LGTM. Should we file a drivers ticket so we can create language tickets to go with it? @durran

@baileympearson baileympearson changed the title test: remove app insights test(DRIVERS-3096): remove app insights Feb 3, 2025
@baileympearson baileympearson changed the title test(DRIVERS-3096): remove app insights test(DRIVERS-3096): remove app insights from deployed lambda tests Feb 3, 2025
baileympearson
baileympearson previously approved these changes Feb 3, 2025
dariakp
dariakp previously requested changes Feb 3, 2025
Copy link
Contributor

@dariakp dariakp left a comment

Choose a reason for hiding this comment

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

Adelin suggested removing ApplicationResourceGroup, too, because it is a companion to ApplicationInsightsMonitoring, or is that inaccurate?

@dariakp
Copy link
Contributor

dariakp commented Feb 3, 2025

@baileympearson @durran Also, it seems like a lot of test failures, do we know what that's all about?

@durran
Copy link
Member Author

durran commented Feb 3, 2025

Adelin suggested removing ApplicationResourceGroup, too, because it is a companion to ApplicationInsightsMonitoring, or is that inaccurate?

I removed that now as well.

@baileympearson
Copy link
Contributor

@baileympearson @durran Also, it seems like a lot of test failures, do we know what that's all about?

No, but they're unrelated.

@dariakp dariakp added Team Review Needs review from team and removed Primary Review In Review with primary reviewer, not yet ready for team's eyes labels Feb 3, 2025
@baileympearson baileympearson dismissed dariakp’s stale review February 4, 2025 18:01

changes are addressed

@baileympearson baileympearson merged commit 58572de into main Feb 4, 2025
24 of 30 checks passed
@baileympearson baileympearson deleted the remove-app-insights branch February 4, 2025 18:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Team Review Needs review from team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants