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

Follow up Kapacitor module enhancements. #46825

Merged
merged 5 commits into from Apr 16, 2018

Conversation

Projects
None yet
3 participants
@AAbouZaid
Copy link
Contributor

commented Apr 3, 2018

What issues does this PR fix or reference?

A follow up for #46294

  • Update versionadded for new conf.
  • Make database name mandatory.

Previous Behavior

Database name was set to None where it was producing an error.

New Behavior

Database name is a mandatory field.

Tests written?

No need.

Commits signed with GPG?

Yes

@rallytime

This comment has been minimized.

Copy link
Contributor

commented Apr 3, 2018

@cachedout
Copy link
Collaborator

left a comment

Approved pending the lint errors being fixed.

@AAbouZaid AAbouZaid force-pushed the AAbouZaid:kapacitor_enhancements branch from 4c4cdfd to 20f4be7 Apr 4, 2018

@@ -154,6 +159,12 @@ def define_task(name,
salt '*' kapacitor.define_task cpu salt://kapacitor/cpu.tick database=telegraf
'''
if not database and not dbrps:

This comment has been minimized.

Copy link
@AAbouZaid

AAbouZaid Apr 4, 2018

Author Contributor

@cachedout Since we cannot change the parameter directly from optional to mandatory to avoid breaking anything, what do you think about this approach? 🤔

This comment has been minimized.

Copy link
@cachedout

cachedout Apr 9, 2018

Collaborator

If we do this we should also log an error with log.error() IMHO

@rallytime

This comment has been minimized.

Copy link
Contributor

commented Apr 9, 2018

re-run py

Mike Place
@rallytime

This comment has been minimized.

Copy link
Contributor

commented Apr 9, 2018

@AAbouZaid It looks like your change is causing the following test to fail:

  • unit.modules.test_kapacitor.KapacitorTestCase.test_define_task

https://jenkins.saltstack.com/job/PR/job/salt-pr-linode-cent7-py3/3893/testReport/junit/unit.modules.test_kapacitor/KapacitorTestCase/test_define_task/

Can you take a look?

@cachedout

This comment has been minimized.

Copy link
Collaborator

commented Apr 12, 2018

Bump @AAbouZaid. Will you be able to investigate the failing test?

@AAbouZaid

This comment has been minimized.

Copy link
Contributor Author

commented Apr 13, 2018

@cachedout I'm checking it right now, thanks.

@AAbouZaid

This comment has been minimized.

Copy link
Contributor Author

commented Apr 15, 2018

@cachedout everything looks fine now 🎉

@cachedout cachedout merged commit 5852993 into saltstack:develop Apr 16, 2018

5 of 10 checks passed

jenkins/PR/salt-pr-rs-cent7-n Pull Requests » Salt PR - RS CentOS 7 #18099 — ABORTED
Details
default Build finished.
Details
jenkins/PR/salt-pr-linode-cent7-py3 Pull Requests » Salt PR - Linode CentOS 7 - PY3 #4053 — FAILURE
Details
jenkins/PR/salt-pr-linode-ubuntu14-n Pull Requests » Salt PR - Linode Ubuntu14.04 #21822 — FAILURE
Details
jenkins/PR/salt-pr-linode-ubuntu16-py3 Pull Requests » Salt PR - Linode Ubuntu16.04 - PY3 #8888 — FAILURE
Details
WIP ready for review
Details
codeclimate All good!
Details
jenkins/PR/salt-pr-clone Pull Requests » Salt PR - Clone #24217 — SUCCESS
Details
jenkins/PR/salt-pr-docs-n Pull Requests » Salt PR - Docs #16371 — SUCCESS
Details
jenkins/PR/salt-pr-lint-n Pull Requests » Salt PR - Code Lint #20990 — SUCCESS
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.