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

Create job to report build status to SCM via SCMStatusReporter #11124

Merged
merged 1 commit into from
May 28, 2021
Merged

Create job to report build status to SCM via SCMStatusReporter #11124

merged 1 commit into from
May 28, 2021

Conversation

vpereira
Copy link
Contributor

@vpereira vpereira commented May 11, 2021 •
edited by saraycp

Extracted from #11060

If this PR requires any particular action or consideration before deployment,
please check the reasons or add your own to the list:

  • Requires a database migration that can cause downtime. Postpone deployment until maintenance window[1].
  • Contains a data migration that can cause temporary inconsistency, so should be run at a specific point of time.
  • Changes some configuration files (e.g. options.yml), so the changes have to be applied manually in the reference server.
  • A new Feature Toggle[2] should be enabled in the reference server.
  • Proper documentation or announcement has to be published upfront since the introduced changes can confuse the users.

[1] https://github.com/openSUSE/open-build-service/wiki/Deployment-of-build.opensuse.org#when-there-are-migrations
[2] https://github.com/openSUSE/open-build-service/wiki/Feature-Toggles-%28Flipper%29#you-want-real-people-to-test-your-feature

@dmarcoux dmarcoux added the Frontend Things related to the OBS RoR app label May 11, 2021
@dmarcoux
Copy link
Contributor

Write/adapt specs

@vpereira vpereira force-pushed the report_to_scm_job branch 2 times, most recently from e2f4532 to ed3125b Compare May 12, 2021 09:51
@danidoni danidoni force-pushed the report_to_scm_job branch 7 times, most recently from 9f85d8b to 82de217 Compare May 21, 2021 12:37
@danidoni danidoni force-pushed the report_to_scm_job branch 4 times, most recently from b92aef4 to 627472a Compare May 25, 2021 10:40
hennevogel
hennevogel requested changes May 25, 2021
View reviewed changes
src/api/app/jobs/report_to_scm_job.rb Outdated Show resolved Hide resolved
src/api/app/jobs/report_to_scm_job.rb Outdated Show resolved Hide resolved
@danidoni danidoni force-pushed the report_to_scm_job branch 2 times, most recently from ff2013c to 349f692 Compare May 25, 2021 14:25
krauselukas added a commit to krauselukas/open-build-service that referenced this pull request May 25, 2021
@krauselukas
Remove package_id from event table
6282c19 
After changing the from the attempt of building an association
between an event and a package, to fetching the package
using the passed payload in the `ReportToScmJob`
in openSUSE#11124
the package_id is not longer needed.
krauselukas added a commit to krauselukas/open-build-service that referenced this pull request May 25, 2021
@krauselukas
Remove package_id from event table
daebf48 
After changing the from the attempt of building an association
between an event and a package, to fetching the package
using the passed payload in the `ReportToScmJob`
in openSUSE#11124
the package_id is not longer needed.
@krauselukas krauselukas mentioned this pull request May 25, 2021
Merged
krauselukas added a commit to krauselukas/open-build-service that referenced this pull request May 25, 2021
@krauselukas
Remove package_id from event table
6f8bb50 
After changing the from the attempt of building an association
between an event and a package, to fetching the package
using the passed payload in the `ReportToScmJob`
in openSUSE#11124
the package_id is not longer needed.
@danidoni danidoni marked this pull request as draft May 26, 2021 10:40
@danidoni danidoni marked this pull request as ready for review May 27, 2021 10:21
danidoni
danidoni reviewed May 27, 2021
View reviewed changes
src/api/spec/jobs/report_to_scm_job_spec.rb
let(:project) { create(:project, name: 'project_1', maintainer: user) }
let(:package) { create(:package, name: 'package_1', project: project) }
let(:repository) { create(:repository, name: 'repository_1', project: project) }
let(:event) { Event::BuildSuccess.create({ project: project.name, package: package.name, repository: repository.name, reason: 'foo' }) }
Copy link
Contributor

@danidoni danidoni May 27, 2021 •
edited

Choose a reason for hiding this comment

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

This event creation triggers a ReportToScmJob run because this after_create and our current active job configuration. So this spec triggers the job twice.

danidoni
danidoni reviewed May 27, 2021
View reviewed changes
src/api/spec/jobs/report_to_scm_job_spec.rb
end

it 'does not call the scm reporter' do
expect_any_instance_of(SCMStatusReporter).not_to receive(:call) # rubocop:disable RSpec/AnyInstance
Copy link
Contributor

Choose a reason for hiding this comment

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

This expectation can be refactored later in another PR using a custom status reporter passed as parameter to the ReportToScmJob.

@krauselukas
Copy link
Contributor

@danidoni I would clean up a bit the commit history, the third commit in this PR (Extend Event classes to track a package) does not really do what it describes anymore. Besided that it looks good to me

saraycp
saraycp reviewed May 27, 2021
View reviewed changes
src/api/spec/jobs/report_to_scm_job_spec.rb
@@ -0,0 +1,29 @@
require 'rails_helper'

RSpec.describe ReportToScmJob, vcr: false do
Copy link
Contributor

Choose a reason for hiding this comment

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

Is vcr set to false on purpose? Could it just be removed?

Copy link
Contributor

Choose a reason for hiding this comment

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

Enabling this just records calls to backend that are not related to the spec. So I think we don't need to enable this.

Copy link
Contributor

Choose a reason for hiding this comment

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

so no need to add the flag, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm working on an improved spec, I will take care of that there

@danidoni
Copy link
Contributor

@danidoni I would clean up a bit the commit history, the third commit in this PR (Extend Event classes to track a package) does not really do what it describes anymore. Besided that it looks good to me

I will squash them all into just one commit.

@danidoni danidoni merged commit 45159fb into openSUSE:master May 28, 2021
@vpereira vpereira deleted the report_to_scm_job branch July 22, 2021 08:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danidoni danidoni danidoni left review comments

@hennevogel hennevogel hennevogel requested changes

@dmarcoux dmarcoux dmarcoux left review comments

@saraycp saraycp saraycp left review comments

@krauselukas krauselukas krauselukas 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.

None yet
6 participants
@vpereira @dmarcoux @krauselukas @danidoni @hennevogel @saraycp