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(actions): send metrics on pr and send none values #4058

Merged
merged 10 commits into from
Nov 6, 2023

Conversation

haverchuck
Copy link
Contributor

Issue #, if available:
Fixes metrics issues

Description of changes:

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@haverchuck haverchuck requested a review from a team as a code owner November 2, 2023 22:39
@@ -7,7 +7,7 @@ environment:
sdk: ^3.2.0-150.0.dev

dependencies:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For some reason, the github action was pulling in a very old version of aws_common that was missing a Stream Extension we were relying on. Bumping the version to avoid a slew of "method not supported on Stream" exceptions.

NikaHsn
NikaHsn previously approved these changes Nov 3, 2023
Copy link
Contributor

@fjnoyp fjnoyp left a comment

Choose a reason for hiding this comment

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

Thanks looks good left some comments

@@ -53,14 +53,6 @@ inputs:
runs:
using: "composite"
steps:
- name: Exit if not scheduled
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you sure you want to track metrics for every single test trigger?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, at least for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

If we do this shouldn't we track the trigger type? Otherwise we have no means to separate schedules from PR tests that are failing because the PR is half done.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh that's a great point - let me think on it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done - sending event type now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Where are you sending this event type or other means to distinguish events based on whether they were triggered by a PR / scheduled / etc. ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

@fjnoyp fjnoyp left a comment

Choose a reason for hiding this comment

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

Where do you send the events test type to distinguish based on where they were triggered? Otherwise looks good!

@fjnoyp fjnoyp self-requested a review November 6, 2023 16:48
@haverchuck haverchuck merged commit 0bc1446 into main Nov 6, 2023
9 checks passed
@NikaHsn NikaHsn deleted the fix/actions-metrics branch August 21, 2024 23:05
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