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

[MM-62303] workflows/perf: add new workflow to be triggered by a label #8426

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

isacikgoz
Copy link
Member

Summary

This PR is to start a discussion around how we are going to publish performance metrics on PRs. Right now, as discussed we are triggering a workflow (right now it's only the test action).

We have 2 alternatives moving forward;
a. Trigger another workflow (or a simple REST API request) within the mobile app to publish metrics as a comment
b. Persist result in the runner and pass the output to another workflow (which will be creating the comment

I think we were agreed on the alternative a, I'd prefer the other option (b) but it's up to whichever be the best for your tast @enahum :)

Ticket Link

https://mattermost.atlassian.net/browse/MM-62303

Checklist

  • Added or updated unit tests (required for all new features)
  • Has UI changes
  • Includes text changes and localization file updates
  • Have tested against the 5 core themes to ensure consistency between them.
  • Have run E2E tests by adding label E2E iOS tests for PR.

Release Note

NONE

@isacikgoz isacikgoz added the 2: Dev Review Requires review by a core commiter label Dec 18, 2024
@isacikgoz isacikgoz requested a review from enahum December 18, 2024 12:21
@enahum
Copy link
Contributor

enahum commented Dec 18, 2024

Sounds good @isacikgoz lets see how this shapes up..

I also prefer B, but A is much easier to implement as there is no need to open the box, extract and parse the logs to post the message.

Also with option A we can include things that are not in the logs like the metric detail of every request

@isacikgoz
Copy link
Member Author

@enahum I created the blueprint for alternative B, we just need to get the output from the tests and parse it (if necessary) then workflow should create a comment accordingly.

@enahum
Copy link
Contributor

enahum commented Dec 18, 2024

@enahum I created the blueprint for alternative B, we just need to get the output from the tests and parse it (if necessary) then workflow should create a comment accordingly.

Well then if this is the case, you need to tell me how to output that to wherever it needs to as I have no clue at this point.

That is another reason I wanted to go with option A.

Feel free to add the necessary code to create the output you expect as well

@isacikgoz
Copy link
Member Author

Ah, At first I thought that you were in favor of plan B @enahum

Okay, so for plan B, we'll use the REST API. For creating a comment in the PR, you basically need to use this API: https://docs.github.com/en/rest/issues/comments?apiVersion=2022-11-28#create-an-issue-comment

The access token will be passed to the runner via ${{secrets.GITHUB_TOKEN}} (it's up to us to name it actually), then the other fields such as issue number, organization, repository will be passed to the runner as environment variables. We can do the same thing, naming will be up to us.

If this approach is simpler, we can do with 63cf90c

Well then if this is the case, you need to tell me how to output that to wherever it needs to as I have no clue at this point.

Just to clarify this; It's not a certain place, you just set it here:

path: pathToTestResults # Path to test results saved from the performance test

@enahum
Copy link
Contributor

enahum commented Dec 22, 2024

@rahimrahman you are going to have to take care of this one :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2: Dev Review Requires review by a core commiter release-note-none
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants