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.

Sign up for GitHub

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

Jump to bottom

Expand workflow run with payload info #14527

Merged
merged 4 commits into from
Jun 26, 2023
Merged

Expand workflow run with payload info #14527

merged 4 commits into from
Jun 26, 2023
+191 −261

Conversation

saraycp
Copy link
Contributor

@saraycp saraycp commented Jun 21, 2023 •
edited by hennevogel
Loading

Extract some of the important information that is in the SCM webhook HTTP request (headers/body) into WorkflowRun attributes so they are easier to query.

@github-actions github-actions bot added the Frontend Things related to the OBS RoR app label Jun 21, 2023
@hennevogel hennevogel force-pushed the expand_workflow_run_with_payload_info branch 2 times, most recently from 8fa7b63 to f42caf2 Compare June 21, 2023 15:00
@hennevogel hennevogel marked this pull request as ready for review June 21, 2023 15:00
@danidoni
Copy link
Contributor

danidoni commented Jun 22, 2023 •
edited
Loading

I'm not sure about the benefits of extracting that extractor class... Can we leave that for another PR? Maybe then we can expand that refactoring further...

@hennevogel
Copy link
Member

I'm not sure about the benefits of extracting that extractor class...

What are you unsure about?

@hennevogel hennevogel force-pushed the expand_workflow_run_with_payload_info branch 4 times, most recently from ce57d59 to 3bea712 Compare June 22, 2023 11:06
@danidoni
Copy link
Contributor

What are you unsure about?

We extract that class, but we don't use it anywhere else. I don't see any benefit in moving that code to it's own class.

@hennevogel
Copy link
Member

I don't see any benefit in moving that code to it's own class.

Smaller class, easier re-use (see the data migration for instance).

@hennevogel hennevogel force-pushed the expand_workflow_run_with_payload_info branch from 3bea712 to ec0d62b Compare June 22, 2023 17:49
@hennevogel hennevogel marked this pull request as draft June 22, 2023 17:49
@hennevogel hennevogel force-pushed the expand_workflow_run_with_payload_info branch 2 times, most recently from 5d3fe6d to 40d85a1 Compare June 22, 2023 20:11
@hennevogel hennevogel marked this pull request as ready for review June 22, 2023 20:17
@hennevogel hennevogel force-pushed the expand_workflow_run_with_payload_info branch 2 times, most recently from fc30575 to de976c5 Compare June 22, 2023 21:11
@codecov
Copy link

codecov bot commented Jun 22, 2023 •
edited
Loading

Codecov Report

Merging #14527 (7c3488b) into master (4b4cf5a) will increase coverage by 0.03%.
The diff coverage is 90.16%.

❗ Current head 7c3488b differs from pull request most recent head 9f9929c. Consider uploading reports for the commit 9f9929c to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #14527      +/-   ##
==========================================
+ Coverage   87.18%   87.22%   +0.03%     
==========================================
  Files         741      742       +1     
  Lines       24970    24954      -16     
==========================================
- Hits        21771    21765       -6     
+ Misses       3199     3189      -10

@danidoni
Copy link
Contributor

danidoni commented Jun 23, 2023 •
edited
Loading

Smaller class, easier re-use (see the data migration for instance).

Which data migration? I'm a bit lost here... I only see it being used here

@hennevogel hennevogel force-pushed the expand_workflow_run_with_payload_info branch from de976c5 to c548bf1 Compare June 23, 2023 09:21
@hennevogel
Copy link
Member

Which data migration?

It's gone by now. So is the service.

@danidoni danidoni force-pushed the expand_workflow_run_with_payload_info branch from c548bf1 to c02c36d Compare June 23, 2023 10:44
@hennevogel hennevogel added Reference Server 🖥️ Things related to build.opensuse.org DO NOT MERGE ⚠️ Explain yourself if you add/remove this label to a PR labels Jun 23, 2023
@hennevogel
Copy link
Member

Do not merge because it contains an "unsafe" data migration that probably needs to be run in the maintenance window

@hennevogel hennevogel mentioned this pull request Jun 23, 2023
Merged
@hennevogel hennevogel force-pushed the expand_workflow_run_with_payload_info branch from 7c3488b to 8daf783 Compare June 23, 2023 12:41
@hennevogel hennevogel removed Reference Server 🖥️ Things related to build.opensuse.org DO NOT MERGE ⚠️ Explain yourself if you add/remove this label to a PR labels Jun 23, 2023
@hennevogel
Copy link
Member

Managed to move the migrations out to #14541

@hennevogel hennevogel force-pushed the expand_workflow_run_with_payload_info branch from 8daf783 to 255ac12 Compare June 23, 2023 12:50
@hennevogel hennevogel marked this pull request as draft June 23, 2023 12:55
@hennevogel hennevogel force-pushed the expand_workflow_run_with_payload_info branch 2 times, most recently from edd1dee to 4af5795 Compare June 23, 2023 15:31
@saraycp @hennevogel
Drop WorkflowRun custom getters
f0f87af 
We have the data in the attributes by now.
@hennevogel hennevogel force-pushed the expand_workflow_run_with_payload_info branch from 4af5795 to 1c45d3f Compare June 26, 2023 09:25
@hennevogel hennevogel marked this pull request as ready for review June 26, 2023 09:26
saraycp and others added 3 commits June 26, 2023 12:57
@saraycp @hennevogel
Introduces HTTP requests extractor concerns
06cd51b 
Two controller concerns to extract data from the HTTP requests to fill
`WorkflowRun` attributes.
@hennevogel
Cleanup WorkflowRun Factory
af3bbec 
- there is a functioning default (:workflow_run)
- with traits for various events and states
- there is a SCM vendor (gitlab) specific factory that builds on top of the
  default
@hennevogel
Adopt specs to WorkflowRun factory changes
9f9929c
@hennevogel hennevogel force-pushed the expand_workflow_run_with_payload_info branch from 1c45d3f to 9f9929c Compare June 26, 2023 10:58
@rubhanazeem rubhanazeem self-requested a review June 26, 2023 11:19
@hennevogel hennevogel merged commit bdcf3ec into openSUSE:master Jun 26, 2023
@danidoni danidoni deleted the expand_workflow_run_with_payload_info branch June 26, 2023 13:59
This was referenced Jun 28, 2023
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hennevogel hennevogel hennevogel left review comments

@hellcp-work hellcp-work hellcp-work left review comments

@rubhanazeem rubhanazeem rubhanazeem approved these changes

Assignees
No one assigned
Labels
Frontend Things related to the OBS RoR app
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@saraycp @danidoni @hennevogel @rubhanazeem @hellcp-work