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

feat(job-runner): enable snuba jobs command to run from JSON manifest #6320

Merged
merged 1 commit into from
Sep 18, 2024

Conversation

xurui-c
Copy link
Member

@xurui-c xurui-c commented Sep 18, 2024

@xurui-c xurui-c marked this pull request as ready for review September 18, 2024 04:57
@xurui-c xurui-c requested a review from a team as a code owner September 18, 2024 04:57
Copy link

codecov bot commented Sep 18, 2024

❌ 1 Tests Failed:

Tests completed Failed Passed Skipped
584 1 583 1
View the top 1 failed tests by shortest run time
tests.cli.test_jobs test_json_valid
Stack Traces | 0.002s run time
Traceback (most recent call last):
  File ".../tests/cli/test_jobs.py", line 71, in test_json_valid
    assert result.exit_code == 0
AssertionError: assert 1 == 0
 +  where 1 = <Result FileNotFoundError(2, 'No such file or directory')>.exit_code

To view individual test run time comparison to the main branch, go to the Test Analytics Dashboard

Copy link
Member

@onkar onkar left a comment

Choose a reason for hiding this comment

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

LGTM overall, left a few nits.

Comment on lines 34 to 38
params = {}
for pair in pairs:
k, v = pair.split("=")
kwargs[k] = v
params[k] = v
return params
Copy link
Member

Choose a reason for hiding this comment

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

nit: We can use dict comprehension here. Something like this:
return {k: v for k, v in (pair.split("=") for pair in pairs)}

Comment on lines 10 to 12
if job_type_class is None:
raise Exception(
f"Job does not exist. Did you make a file {class_name}.py yet?"
f"Job does not exist. Did you make a file {job_spec.job_type}.py yet?"
Copy link
Member

Choose a reason for hiding this comment

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

Does it make sense to create a custom exception instead of the base Exception?

import click

from snuba.manual_jobs import Job
from snuba.cli import logger
Copy link
Member

Choose a reason for hiding this comment

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

nit: Not a big deal at this point but do you think we should have a separate logger for manual_jobs? The second answer to this question might be something we want to have, or we can follow the pattern from other modules.

@xurui-c xurui-c merged commit f030a4d into master Sep 18, 2024
30 checks passed
@xurui-c xurui-c deleted the rachel/manifest branch September 18, 2024 20:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants