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

WIP: Refactoring SCMWebhook away #15956

Open
wants to merge 66 commits into
base: master
Choose a base branch
from
Open

WIP: Refactoring SCMWebhook away #15956

wants to merge 66 commits into from

Conversation

hennevogel
Copy link
Member

No description provided.

@github-actions github-actions bot added the Frontend Things related to the OBS RoR app label Apr 10, 2024
@hennevogel hennevogel marked this pull request as draft April 10, 2024 10:17
@hennevogel hennevogel force-pushed the refactoring/scm_webhook branch 2 times, most recently from b848e29 to d9d43cb Compare April 10, 2024 13:57
@krauselukas krauselukas force-pushed the refactoring/scm_webhook branch 4 times, most recently from 167b33c to 0d9c7ab Compare April 11, 2024 12:59
@hennevogel
Copy link
Member Author

Merge SCMWebhook into WorkflowRun (L)

@krauselukas krauselukas force-pushed the refactoring/scm_webhook branch 2 times, most recently from ce4dfbb to 940a867 Compare April 12, 2024 12:47
@hennevogel hennevogel force-pushed the refactoring/scm_webhook branch 5 times, most recently from 1737385 to 70217b1 Compare April 12, 2024 14:56
@eduardoj
Copy link
Member

Typo in one of the commits title: "Regernerate database_consistency todo" should be "Regenerate database_consistency todo"

krauselukas and others added 22 commits July 1, 2024 16:46
@krauselukas @saraycp
Adapt trigger_services_spec
3ed23c0
@krauselukas @saraycp
Adapt workflow/step_spec.spec
7ba8abf
@krauselukas @saraycp
Couple of fixes and additions
80fb171
@krauselukas @saraycp
Adapt trigger_workflow_controller_spec
f6390b1
@krauselukas @saraycp
Adapt order of callbacks and validations in trigger workflow controller
7e34ee8
@saraycp
Adapt Workflow model spec
d1da657
@saraycp
Adapt Workflows::ArtifactsCollector service spec
ab828e7
@saraycp
Adapt WorkflowFiltersValidator spec
9674cf9
@krauselukas @saraycp
Adapt workflow_configuration_url_valid_and_accessible validation me…
e1540f0 
…thod

We have to pass it a `WorkflowRun` object, otherwise its failing...
@krauselukas @saraycp
Adapt token workflow spec
02a13ff
@saraycp
Adapt Workflows::YAMLToWorkflowsService spec
e8d583e
@saraycp
Remove duplication of target_package_name method
6c92d1b 
Adapt Workflow::Step#target_package_name to avoid duplication and remove
the implementation in Workflow::Step::SetFlags.
@krauselukas @saraycp
Adapt yaml downloader and its spec
02daa19
@krauselukas @saraycp
Adapt naming of variables and add missing methods for gitea payload
e9463a6
@krauselukas @saraycp
Consider gitea in workflow run model validations
818939b
@krauselukas @saraycp
Set workflow run attributes before validation and only on create
fe1f6f1 
We have to make sure the attributes are set before the validation
happens to catch eventual errors. We also have to make sure that
the attributes are only set from the request payload if they havent
been set explicitly when creating the record.
@krauselukas @saraycp
Handle validation errors comming from workflow run
b582481 
We can simply handle the active record exceptions and render the
appropriate error. Also, we can authorize first the token, there is no
need to even initialize a workflow run if a user is not permitted to run
a workflow.

Also get rid of the create_workflow_run method. The workflow creation is
moved to the 'create' method but the request_headers extraction is moved
to a specific method (to not increase complexity).
@saraycp
Remove no longer needed data migrations
c2576e3 
These migrations and specs need adaptation that is not worth it.
The data migrations were applied to production long ago. It's safe to
remove it now.

- BackfillScmVendorAndHookEventInWorkflowRun. Workflow run has a new validation that doesn't allow to create it
without scm_vendor.
- RemoveUnsupportedHookEvents. The constants are moved to another model.
@krauselukas @saraycp
Remove dropped SCMWebhook class names from zeitwerk initializer
1d2543a
@krauselukas @saraycp
Drop SCMWebhook from workflow_artifacts_per_step_component_preview
6d2b7f4
@krauselukas @saraycp
Always use merge_request as object_kind for _branch_request file
4b8032c 
Thats what the TAR SCM service expects, there are not other types
that are supported....
@saraycp
Adapt SCMStatusReporter and the related spec
1406429 
Now the ReportToSCMJob needs to have the workflow run set.
@krauselukas krauselukas marked this pull request as ready for review July 1, 2024 14:59
@krauselukas krauselukas changed the title Refactoring SCMWebhook away WIP: Refactoring SCMWebhook away Jul 1, 2024
@saraycp
Stop using subscription payload in reports
5aaf4ac 
Those values are now defined in workflow_run, so we need to set the
workflow run in all the specs too.
krauselukas
krauselukas reviewed Jul 2, 2024
View reviewed changes
src/api/app/services/github_status_reporter.rb
commit_sha: @event_subscription_payload[:commit_sha],
api_endpoint: @workflow_run.api_endpoint,
target_repository_full_name: @workflow_run.target_repository_full_name,
commit_sha: @workflow_run[:commit_sha],
Copy link
Contributor

Choose a reason for hiding this comment

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

should be @workflow_run.commit_sha

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@krauselukas krauselukas krauselukas left review comments

@saraycp saraycp saraycp left review comments

At least 1 approving review is required to merge this pull request.

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.
4 participants
@hennevogel @eduardoj @saraycp @krauselukas