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

add celery tasks, now call these from models #3798

Merged
merged 3 commits into from Mar 6, 2020
Merged

add celery tasks, now call these from models #3798

merged 3 commits into from Mar 6, 2020

Conversation

StephenCoady
Copy link
Contributor

fyi @nphilipp

Signed-off-by: Stephen Coady scoady@redhat.com

@StephenCoady StephenCoady requested a review from a team as a code owner November 25, 2019 11:14
@StephenCoady StephenCoady added the WIP Work in progress label Nov 25, 2019
abompard
abompard previously requested changes Nov 26, 2019
View reviewed changes
Copy link
Member

@abompard abompard left a comment

Choose a reason for hiding this comment

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

Once the functions are moved to tasks, I'll expect some unit tests to fail (because the corresponding work would be done accordingly). I think you'll also have to reorder those parts of the unit tests, possibly create tests for the new tasks in the proper test module.

One more thing, I think it's worth adding an entry in the release notes saying that these actions will now be done asynchronously, please add a news fragment.

bodhi/server/models.py Outdated
# EL6 doesn't have these, and that's okay...
# We still warn in case the config gets messed up.
log.warning('%s has no pending_signing_tag' % up.release.name)
tag_build(new_builds, up)
Copy link
Member

Choose a reason for hiding this comment

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

Celery tasks are called using the .delay() method, please check out how it's done with the handle_update() task.

bodhi/server/services/updates.py Outdated
to_tag = side_tag_signing_pending
# Move every new build to <from_tag>-signing-pending tag
update.add_tag(to_tag)
create_tag(updates, from_tag)
Copy link
Member

Choose a reason for hiding this comment

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

Same comment here, you'll have to use the .delay() method.

bodhi/server/tasks/__init__.py Outdated


@app.task(name="create_tag")
def create_tag(updates: list, from_tag):
Copy link
Member

Choose a reason for hiding this comment

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

Is from_tag supposed to be a string? If so, please type it accordingly.

bodhi/server/tasks/__init__.py Outdated


@app.task(name="tag_build")
def tag_build(builds: list, update):
Copy link
Member

Choose a reason for hiding this comment

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

Please type the update argument.

@mattiaverga
Copy link
Contributor

@abompard should these two new tasks be worked by the has_koji_mount celery queue?

bodhi/celeryconfig.py

Lines 19 to 24 in 983f7c7

# Task routing
task_routes = {
# Route the compose task to a specific queue that will only be run on hosts
# that have a Koji mount.
'compose': {'queue': 'has_koji_mount'},
}

@abompard
Copy link
Member

@abompard should these two new tasks be worked by the has_koji_mount celery queue?

Unless I'm mistaken this code is only using the Koji API, so it can be executed from anywhere, we don't need the actual mount.

@nphilipp
Copy link
Member

nphilipp commented Feb 6, 2020 •
edited

The type hints introduced a cyclic import (models -> tasks -> models) for which I have a fix locally. I noticed, however, that (expectedly) some existing tests block on when calling the new tasks. I'll mock these calls out and instead add unit tests testing create_tag, task_build in isolation modeled after existing tests for Celery tasks.

@nphilipp
Copy link
Member

This fixes the tests breaking because of the introduction of using Celery tasks for Koji tagging. Needs unit tests for the new Celery tasks, though, so still WIP.

@lgtm-com
Copy link

lgtm-com bot commented Feb 11, 2020

This pull request introduces 2 alerts when merging f8fd4c9 into 1be3f91 - view on LGTM.com

new alerts:

  • 2 for Module imports itself

@cverna cverna added this to the 5.2.0 milestone Mar 3, 2020
@lgtm-com
Copy link

lgtm-com bot commented Mar 4, 2020

This pull request introduces 2 alerts when merging 3234d71 into ab024c9 - view on LGTM.com

new alerts:

  • 2 for Module imports itself

@cverna cverna force-pushed the 3061 branch 2 times, most recently from d74f3c2 to 6787158 Compare March 5, 2020 16:31
@lgtm-com
Copy link

lgtm-com bot commented Mar 5, 2020

This pull request introduces 2 alerts when merging 6787158 into 63f9756 - view on LGTM.com

new alerts:

  • 2 for Module imports itself

@StephenCoady @cverna
Handle tagging of builds in celery tasks.
0ee733c 
This commit moves the calls to koji to tag builds from
an update to an async celery task.

Signed-off-by: Stephen Coady <scoady@redhat.com>
Signed-off-by: Nils Philippsen <nils@redhat.com>
Signed-off-by: Clement Verna <cverna@tutanota.com>

Remove obsolete tests & mock out new task handlers

Signed-off-by: Nils Philippsen <nils@redhat.com>

Add unit tests for the new tasks
@cverna cverna dismissed abompard’s stale review March 5, 2020 18:54

comments were addressed

@lgtm-com
Copy link

lgtm-com bot commented Mar 5, 2020

This pull request introduces 2 alerts when merging 23a1609 into 3268af0 - view on LGTM.com

new alerts:

  • 2 for Module imports itself

@lgtm-com
Copy link

lgtm-com bot commented Mar 5, 2020

This pull request introduces 2 alerts when merging d66a0fb into 9c4cfea - view on LGTM.com

new alerts:

  • 2 for Module imports itself

nphilipp
nphilipp approved these changes Mar 6, 2020
View reviewed changes
Copy link
Member

@nphilipp nphilipp left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@cverna cverna added WIP Work in progress and removed WIP Work in progress labels Mar 6, 2020
@mergify mergify bot merged commit 3f045e0 into fedora-infra:develop Mar 6, 2020
@nphilipp nphilipp mentioned this pull request Mar 9, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nphilipp nphilipp nphilipp approved these changes

@abompard abompard abompard left review comments

Assignees
No one assigned
Labels
WIP Work in progress
Projects
None yet
Milestone
5.2.0
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@StephenCoady @mattiaverga @abompard @nphilipp @cverna