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

RHINENG-15237: Update floorist queries and payload file to randomly add different ca… #2186

Merged
merged 1 commit into from
Jan 16, 2025

Conversation

thearifismail
Copy link
Contributor

…nonical_facts

rh-pre-commit.version: 2.3.2
rh-pre-commit.check-secrets: ENABLED

Overview

This PR is being created to address RHINENG-15237.
It also updates the utils/payloads.py file, to generate news hosts by randomly choosing to add provider_id, subscription_manager_id, or bios_uuid. insights_id is always used. Randomization also picks a reporter among ["cloud-connector", "puptoo", "rhsm-conduit", "rhsm-system-profile-bridge", "yuptoo"].

The Floorist queries will be put the results in AWS S3 buckets which will be used for creating metrics.

PR Checklist

  • Keep PR title short, ideally under 72 characters
  • Descriptive comments provided in complex code blocks
  • Include raw query examples in the PR description, if adding/modifying SQL query
  • Tests: validate optimal/expected output
  • Tests: validate exceptions and failure scenarios
  • Tests: edge cases
  • Recovers or fails gracefully during potential resource outages (e.g. DB, Kafka)
  • Uses type hinting, if convenient
  • Documentation, if this PR changes the way other services interact with host inventory
  • Links to related PRs

Secure Coding Practices Documentation Reference

You can find documentation on this checklist here.

Secure Coding Checklist

  • Input Validation
  • Output Encoding
  • Authentication and Password Management
  • Session Management
  • Access Control
  • Cryptographic Practices
  • Error Handling and Logging
  • Data Protection
  • Communication Security
  • System Configuration
  • Database Security
  • File Management
  • Memory Management
  • General Coding Practices

…nonical_facts

rh-pre-commit.version: 2.3.2
rh-pre-commit.check-secrets: ENABLED
Copy link
Contributor

@strider strider left a comment

Choose a reason for hiding this comment

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

LGTM

@kruai
Copy link
Collaborator

kruai commented Jan 15, 2025

@thearifismail I'm OK with these changes, but it feels like they should be 2 separate PRs. Are the changes to the payload file related at all to the Jira this PR is addressing?

@thearifismail
Copy link
Contributor Author

@thearifismail I'm OK with these changes, but it feels like they should be 2 separate PRs. Are the changes to the payload file related at all to the Jira this PR is addressing?

I did think about creating two separate PRs because apparently they are. Since testing floorist queries needed variety in hosts canonical_facts and reporters, I submitted the changes in one PR.

@kruai
Copy link
Collaborator

kruai commented Jan 16, 2025

@thearifismail I'm OK with these changes, but it feels like they should be 2 separate PRs. Are the changes to the payload file related at all to the Jira this PR is addressing?

I did think about creating two separate PRs because apparently they are. Since testing floorist queries needed variety in hosts canonical_facts and reporters, I submitted the changes in one PR.

I wasn't aware that payloads.py was being used to test Floorist queries; if that's the case, this seems reasonable enough to me.

Copy link
Collaborator

@kruai kruai left a comment

Choose a reason for hiding this comment

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

LGTM, but please do not merge this until we're told it's safe to merge PRs again (see this slack thread)

@kruai kruai merged commit 9cdf248 into RedHatInsights:master Jan 16, 2025
12 of 13 checks passed
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.

3 participants