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

[HCP Observability] Add safety checks for TelemetryConfig payload response #17511

Merged
merged 1 commit into from
May 30, 2023

Conversation

Achooo
Copy link
Contributor

@Achooo Achooo commented May 30, 2023

Description

This is a follow-up to #17460

This PR adds safety checks in the event that the Telemetry Gateway is down, or the client does not return an error, we should have the response validated.

Testing & Reproduction steps

  • Add unit tests
  • Run integration k8s test

Links

PR Checklist

  • updated test coverage
  • external facing docs updated
  • appropriate backport labels added
  • not a security concern

@Achooo Achooo requested a review from chapmanc May 30, 2023 15:24
@Achooo Achooo force-pushed the fix-hcp-client-telemetry-cfg-checks branch from 1521434 to f7e83d6 Compare May 30, 2023 15:24
@Achooo Achooo changed the title [HCP Observability] Add safety nil checks [HCP Observability] Add safety checks for TelemetryConfig payload response May 30, 2023
@Achooo Achooo added the backport/1.15 This release series is no longer active on CE. Use backport/ent/1.15. label May 30, 2023
Copy link
Contributor

@chapmanc chapmanc left a comment

Choose a reason for hiding this comment

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

LGTM thanks for making this safer!!

@Achooo Achooo marked this pull request as ready for review May 30, 2023 15:53
Copy link
Member

@jmurret jmurret left a comment

Choose a reason for hiding this comment

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

👏 approving, but please remove the changelog before merging. See additional comment.

@@ -0,0 +1,3 @@
```release-note:fix
Copy link
Member

@jmurret jmurret May 30, 2023

Choose a reason for hiding this comment

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

I would remove the changelog entry since you are making a modification to an unreleased feature. So basically this is a change that customers won't need to know about. Basically, the whole feature should get one changelog entry for the feature being released rather than several smaller updates with every PR.

Please remove this entry file and add the pr/no-changelog label to the PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good! thank you :D

@Achooo Achooo force-pushed the fix-hcp-client-telemetry-cfg-checks branch from dabe913 to f7e83d6 Compare May 30, 2023 16:55
@Achooo Achooo added the pr/no-changelog PR does not need a corresponding .changelog entry label May 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/1.15 This release series is no longer active on CE. Use backport/ent/1.15. pr/no-changelog PR does not need a corresponding .changelog entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants