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

Fix db_cluster_id checks #348

Merged
merged 2 commits into from
Sep 23, 2019
Merged

Fix db_cluster_id checks #348

merged 2 commits into from
Sep 23, 2019

Conversation

rwha
Copy link
Contributor

@rwha rwha commented Jun 17, 2019

Pull Request Checklist

No existing issues found, but resolves an error when using --db-cluster-id

General

  • Update Changelog following the conventions laid out here

  • Update README with any necessary configuration snippets

  • Binstubs are created if needed

  • RuboCop passes

  • Existing tests pass

New Plugins

  • Tests

  • Add the plugin to the README

  • Does it have a complete header as outlined here

Purpose

When an RDS DB Cluster ID is provided instead of an instance ID the check fails due to undefined local variables. This changes two lines to properly parse the --db-cluster-id option.

Known Compatibility Issues

None.

@allankcrain
Copy link

@majormoses Is there anything we can do to help get this pushed through a little faster? We have a client who needs this bug fix (and our setup is such that it would make our lives vastly easier if it's fixed in the main upstream repository instead of fixed in a local fork).

@rwha
Copy link
Contributor Author

rwha commented Sep 23, 2019

fixes #360

Copy link
Member

@majormoses majormoses left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -161,7 +161,7 @@ def find_db_instance(id)

def find_db_cluster_writer(id)
wr = rds.describe_db_clusters(db_cluster_identifier: id).db_clusters[0].db_cluster_members.detect(&:is_cluster_writer).db_instance_identifier
unknown 'DB cluster not found.' if cl.nil?
unknown 'DB cluster not found.' if wr.nil?
Copy link
Member

Choose a reason for hiding this comment

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

I really dislike short undescriptive variable names like this, I know its outside of the scope of change...I will probably go back in afterwards and fix this up.

@majormoses majormoses merged commit fe89e43 into sensu-plugins:master Sep 23, 2019
@rwha rwha deleted the cluster_id branch September 24, 2019 23:18
@jsnod
Copy link

jsnod commented Dec 19, 2020

@majormoses I really need this fix on a new Sensu Go setup, how to we get the latest version pushed to Bonsai? It's currently stuck at 18.4.0: https://bonsai.sensu.io/assets/sensu-plugins/sensu-plugins-aws

@majormoses
Copy link
Member

majormoses commented Dec 19, 2020

@jsnod it looks like the bonsai asset deploy failed silently (exited 0) here: https://travis-ci.org/github/sensu-plugins/sensu-plugins-aws/jobs/643238482#L5268 I will need to spend some time digging into that and will have some time to look in the upcoming week but given the holidays I make no promises on timeline. Can you please open a issue to track and tag myself + @jspaleta in it?

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.

4 participants