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

Feature/check subdue #141

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

Conversation

ValkyrieOps
Copy link
Contributor

Pull Request Checklist

Is this in reference to an existing issue?

General

  • Update Changelog following the conventions laid out here

  • Update README with any necessary configuration snippets

  • Cookstyle (rubocop) passes

  • Rspec (unit tests) passes

  • Inspec (integration tests) passes

New Features

  • Added a Testing Artifact as either an automated test or a manual artifact on the PR.

  • Added documentation for it to the README.md

Purpose

With the release of Sensu Go 6.7 check subdue has been added again. This PR adds that functionality back to the cookbook.

Known Compatibility Issues

Can be added to previous versions of Sensu Go but the spec will not be recognized when the json file is uploaded to the backends.

@ValkyrieOps ValkyrieOps requested a review from a team as a code owner April 22, 2022 16:07
@@ -57,7 +57,7 @@ def check_from_resource
spec['runtime_assets'] = new_resource.runtime_assets if new_resource.runtime_assets
spec['secrets'] = new_resource.secrets if new_resource.secrets
spec['stdin'] = new_resource.stdin
spec['subdue'] = new_resource.subdue if new_resource.subdue
spec['subdues'] = new_resource.subdues if new_resource.subdues
Copy link
Contributor

Choose a reason for hiding this comment

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

are both valid?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In my opinion no, but it's unclear how the folks at Sensu intended to resolve this as the original subdue implementation still appears as null in the CheckConfig reference. See the YML output from their cron example check vs the actual subdues documentation section.

In practice one of my example checks using this new functionality lists both but I'm unable to set the original subdue as anything other than null:

type: Check
api_version: core/v2
metadata:
  created_by: s_sensu
  labels:
    sensu.io/managed_by: sensuctl
  name: check_process
  namespace: default
spec:
  check_hooks: null
  command: check_process.rb
  env_vars: null
  executed: 0
  handlers:
  high_flap_threshold: 0
  history: null
  interval: 15
  is_silenced: false
  issued: 0
  last_ok: 0
  low_flap_threshold: 0
  occurrences: 0
  occurrences_watermark: 0
  output: ""
  output_metric_format: ""
  output_metric_handlers: null
  pipelines: []
  proxy_entity_name: ""
  publish: true
  round_robin: false
  runtime_assets:
  - runtime_ruby
  scheduler: ""
  secrets: null
  status: 0
  stdin: false
  subdue: null
  subdues:
  - begin: "2022-04-18T16:00:00-05:00"
    end: "2022-04-18T17:00:00-05:00"
    repeat:
    - weekdays
  - begin: "2022-04-22T16:00:00-05:00"
    end: "2022-04-22T17:00:00-05:00"
    repeat:
    - fridays
  - begin: "2022-04-23T00:00:00-05:00"
    end: "2022-04-23T23:59:59-05:00"
    repeat:
    - saturdays
  - begin: "2022-04-24T00:00:00-05:00"
    end: "2022-04-24T17:00:00-05:00"
    repeat:
    - sundays
  subscriptions:
  - check_process
  timeout: 0
  total_state_change: 0
  ttl: 0

@@ -28,6 +28,7 @@

resource_name :sensu_active_directory
provides :sensu_active_directory
unified_mode true
Copy link
Contributor

Choose a reason for hiding this comment

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

what is the min version of chef is required to use this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unified Mode is unavailable prior to 14.14 according to the docs. I've been adding these to appease cookstyle in my dev environment but isn't strictly necessary for this change.

@@ -44,7 +45,7 @@
property :runtime_assets, Array
property :secrets, Array
property :stdin, [true, false], default: false
property :subdue, Hash
property :subdues, Array
Copy link
Contributor

Choose a reason for hiding this comment

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

if they are both supported then we should add rather than change an interface

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See my previous comment. I wanted to avoid confusion by aligning with the actual Check Resource name.

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.

2 participants