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

Reinstate assume-role functionality (-R) #302

Merged
merged 3 commits into from
Sep 27, 2020

Conversation

pmiles
Copy link
Contributor

@pmiles pmiles commented Oct 30, 2018

Pull Request Checklist

This is in response to issue : #301

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

Known Compatibility Issues

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.

Thanks for your contribution to Sensu plugins! Without people like you submitting PRs we couldn't run the project. Looks like CI is failing can you look at fixing those up? If you need help let me know.

CI may be backed up by enabling https://dependabot.com to help us with a lot of the maintenance work involved in keeping this project working so I can spend more time adding features and fixing bugs.

I tried looking through recent history and can't seem to find this feature in this check, what version were you using that supported that? I am trying to understand if it was an unintended consequence or an intentional decision. I am wondering if it makes sense to add that support to the aws_config library method so that it can be easily added to other checks and reduce duplication across checks.

CHANGELOG.md Outdated
@@ -5,6 +5,10 @@ This CHANGELOG follows the format listed [here](https://github.com/sensu-plugins

## [Unreleased]

## [12.3.0] - 2018-09-25
Copy link
Member

Choose a reason for hiding this comment

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

In general the versioning and dating are left to the maintainers for several reasons:

  • we may disagree on the impact of the change and therefore how its versioned (there is already a 12.4.0 released)
  • we date the release rather then when the contribution was submitted, PR review and acceptance has no SLA and is not guaranteed to be merged within the same day
  • PRs are not a FIFO queue, as such versioning them prior to acceptance leads to needing to ask people to rebase more often

@majormoses
Copy link
Member

Sorry this fell off my radar, I think that rather than adding it to a specific check it should probably be a common option that could be added to everything and included as a lib an example of this pattern can be found in redis plugin:

@majormoses
Copy link
Member

I never heard back but I guess we can merge it for now and refactor later.

@majormoses majormoses merged commit a6c66dc into sensu-plugins:master Sep 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants