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

Initial Version to Monitor Github workflows using the automation app #4

Merged
merged 5 commits into from
Sep 26, 2024

Conversation

prudhvigodithi
Copy link
Collaborator

@prudhvigodithi prudhvigodithi commented Sep 25, 2024

Description

Initial Version to Monitor Github workflows:

  • Onboard workflow to capture the pull_request.closed and workflow_run.completed events.
  • Onboard the github-merged-pulls-monitor and github-workflow-runs-monitor to parse the events and index to the metrics cluster.
  • Add utility class OpensearchClient to use AwsSigv4Signer and connect to the metrics cluster.
  • Update the OperationConfig to support the new configSchema for args.
  • Added the missing license header for the code files and added .licenserc.json.
  • Tests for OpensearchClient and github-merged-pulls-monitor and github-workflow-runs-monitor.
  • Updated the build.yml to build-test.yml to include some testing during the PR creation.

Issues Resolved

Part of opensearch-project/opensearch-build#4941 (comment)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Prudhvi Godithi <pgodithi@amazon.com>
Signed-off-by: Prudhvi Godithi <pgodithi@amazon.com>
@dblock
Copy link
Member

dblock commented Sep 25, 2024

I think all of this needs tests at a minimum. There's no way we're going to have an app where thousands of lines of code gets deployed across all repos without some assurance that they will actually work.

@prudhvigodithi
Copy link
Collaborator Author

I agree with you @dblock, I'm waiting for Peter to define a test workflow in this repo, we should add tests and proper code coverage before we switch to public.
Thanks
@peterzhuamazon

@peterzhuamazon
Copy link
Member

I think all of this needs tests at a minimum. There's no way we're going to have an app where thousands of lines of code gets deployed across all repos without some assurance that they will actually work.

Yes DB, I totally agree we need test here.
Once we get it public tomorrow we will start adding tests to it before adding more functions.

Thanks.

@prudhvigodithi
Copy link
Collaborator Author

prudhvigodithi commented Sep 26, 2024

Added few tests for OpensearchClient in test/utility/opensearchclient.test.ts, @peterzhuamazon please let me know if we want to have testing done this way or any specific framework to be used for this automation app.
Thank you

Signed-off-by: Prudhvi Godithi <pgodithi@amazon.com>
@peterzhuamazon
Copy link
Member

Added few tests for OpensearchClient in test/utility/opensearchclient.test.ts, @peterzhuamazon please let me know if we want to have testing done this way or any specific framework to be used for this automation app. Thank you

Thanks @prudhvigodithi we will be following instructions here:
https://probot.github.io/docs/testing/

Some examples you can check here:
https://github.com/wip/app/tree/master/test
https://github.com/dcoapp/app/tree/main/test

I will do some research and make tweaks based on your changes as well.
Thanks.

src/app.ts Show resolved Hide resolved
src/call/github-merged-pulls-monitor.ts Outdated Show resolved Hide resolved
src/call/github-merged-pulls-monitor.ts Show resolved Hide resolved
src/config/operation-config.ts Show resolved Hide resolved
src/call/github-workflow-runs-monitor.ts Outdated Show resolved Hide resolved
src/call/github-workflow-runs-monitor.ts Outdated Show resolved Hide resolved
src/config/operation-config.ts Show resolved Hide resolved
src/utility/opensearchclient.ts Outdated Show resolved Hide resolved
test/utility/opensearchclient.test.ts Outdated Show resolved Hide resolved
@prudhvigodithi
Copy link
Collaborator Author

Thanks @peterzhuamazon will take a look at your comments, for now added .licenserc.json.

@prudhvigodithi
Copy link
Collaborator Author

@peterzhuamazon addressed your comments and updated the build.yml to build-test.yml to include some testing during the PR creation, please take a look.

Signed-off-by: Peter Zhu <zhujiaxi@amazon.com>
@prudhvigodithi
Copy link
Collaborator Author

Added two new tests github-merged-pulls-monitor.test.ts and github-workflow-runs-monitor.test.ts.

@prudhvigodithi prudhvigodithi changed the title Initial Version to Monitor Github workflows Initial Version to Monitor Github workflows using the automation app Sep 26, 2024
Copy link
Member

@peterzhuamazon peterzhuamazon left a comment

Choose a reason for hiding this comment

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

Thanks @prudhvigodithi .

I will send a follow up PR to address a few things mentioned here.

Thanks!

@peterzhuamazon peterzhuamazon merged commit f194866 into opensearch-project:main Sep 26, 2024
6 checks passed
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