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: pagination error on cloudwatch plugin #9693

Merged
merged 3 commits into from
Sep 14, 2021

Conversation

Doron-Bargo
Copy link
Contributor

@Doron-Bargo Doron-Bargo commented Aug 30, 2021

Fix the error of InvalidParameterValue: Parameters do not match original request parameters as the current code dont use correctly the return token from aws client

Required for all PRs:

  • Updated associated README.md.
  • Wrote appropriate unit tests.
  • Pull request title or commits are in conventional commit format (e.g. feat: or fix:)

Following the discussion on PR #9386 (the update to support multiple namespaces in cloudwatch input plugin ) the latest change keep the token pagination to the next namespace which result in an error of InvalidParameterValue: Parameters do not match original request parameters. This change is to fix this error.

I moved the namespace loop and the aws param initialization to be outside the loop of the pagination

Fix the error of InvalidParameterValue: Parameters do not match original request parameters as the current code dont use correctly the return token from aws client
@telegraf-tiger
Copy link
Contributor

Thanks so much for the pull request!
🤝 ✒️ Just a reminder that the CLA has not yet been signed, and we'll need it before merging. Please sign the CLA when you get a chance, then post a comment here saying !signed-cla

@telegraf-tiger telegraf-tiger bot added the feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin label Aug 30, 2021
@akrantz01 akrantz01 added area/aws AWS plugins including cloudwatch, ecs, kinesis fix pr to fix corresponding bug and removed feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin labels Aug 30, 2021
Copy link
Contributor

@akrantz01 akrantz01 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 the PR! Please make sure you sign the CLA, then comment !signed-cla.

Overall the code looks good. I just left a comment on one block that could be simplified. Also, run gofmt before you commit, so all the code looks uniform.

plugins/inputs/cloudwatch/cloudwatch.go Outdated Show resolved Hide resolved
@Doron-Bargo
Copy link
Contributor Author

!signed-cla

@sspaink
Copy link
Contributor

sspaink commented Sep 7, 2021

@Doron-Bargo looks like the CI is failing due to a minor formatting issue, you can resolve it by running go fmt ./plugins/inputs/cloudwatch in the root of the Telegraf project.

Thanks for digging into this issue and finding a fix!

Copy link
Contributor

@sspaink sspaink left a comment

Choose a reason for hiding this comment

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

lgtm!

@sspaink sspaink added the ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review. label Sep 13, 2021
Copy link
Member

@helenosheaa helenosheaa left a comment

Choose a reason for hiding this comment

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

lgtm

@helenosheaa helenosheaa merged commit 646273a into influxdata:master Sep 14, 2021
reimda pushed a commit that referenced this pull request Sep 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/aws AWS plugins including cloudwatch, ecs, kinesis fix pr to fix corresponding bug ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants