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.

Sign up for GitHub

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

Jump to bottom

refactor(RHINENG-12780): Enhance delete events #2111

Open
wants to merge 14 commits into
base: master
Choose a base branch
from
Open

refactor(RHINENG-12780): Enhance delete events #2111

wants to merge 14 commits into from

Conversation

FabriciaDinizRH
Copy link
Contributor

@FabriciaDinizRH FabriciaDinizRH commented Dec 4, 2024 •
edited
Loading

Overview

This PR is being created to address RHINENG-12780.
Adds subscription_manager_id to delete events. The correct logic for manual_delete is going to be added in a following PR

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

@FabriciaDinizRH FabriciaDinizRH requested a review from a team as a code owner December 4, 2024 18:29
thearifismail
thearifismail reviewed Dec 4, 2024
View reviewed changes
api/host.py Outdated
@@ -243,6 +243,7 @@ def delete_hosts_by_filter(


def _delete_host_list(host_id_list, rbac_filter):
request_host = flask.request.headers.get("Host", "")
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the request_host value, if I make a REST API call from my laptop?
Also suggest using requesting_host to leave out any ambiguity.

api/host.py Outdated
@@ -312,6 +314,10 @@ def delete_host_by_id(host_id_list, rbac_filter=None):
return flask.Response(None, HTTPStatus.OK)


def check_manual_deletion(origin_host: str = ""):
return origin_host in ["console.stage.redhat.com", "console.redhat.com"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggest using origin_host.lower(), just in case CONSOLE.* or Console.* is used by the the request.

app/queue/events.py Outdated
@@ -76,6 +76,8 @@ class HostDeleteEvent(Schema):
org_id = fields.Str()
insights_id = fields.Str()
request_id = fields.Str()
subscription_manager_id = fields.Str()
Copy link
Contributor

Choose a reason for hiding this comment

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

Should start using fields.UUID() for all fields with value type UUID.

jpramos123
jpramos123 reviewed Dec 4, 2024
View reviewed changes
api/host.py Outdated
@@ -243,6 +243,7 @@ def delete_hosts_by_filter(


def _delete_host_list(host_id_list, rbac_filter):
request_host = flask.request.headers.get("Host", "")
Copy link
Contributor

Choose a reason for hiding this comment

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

This may be renamed to another thing? My understand is that this value would be the consoleDOT URL?

Suggested change
request_host = flask.request.headers.get("Host", "")
request_host_url = flask.request.headers.get("Host", "")

Copy link
Collaborator

Choose a reason for hiding this comment

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

@jpramos123 I think it's just the hostname of the request, so maybe request_hostname?

Copy link
Contributor

Choose a reason for hiding this comment

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

@kruai Yup, makes sense

@jpramos123 jpramos123 force-pushed the refactor-delet-events branch from 00f4220 to 279ee52 Compare December 5, 2024 14:18
@jpramos123 jpramos123 force-pushed the refactor-delet-events branch from 279ee52 to 0d4b216 Compare December 5, 2024 14:53
kruai
kruai requested changes Dec 5, 2024
View reviewed changes
api/host.py Outdated
@@ -312,6 +314,10 @@ def delete_host_by_id(host_id_list, rbac_filter=None):
return flask.Response(None, HTTPStatus.OK)


def check_manual_deletion(origin_host: str = ""):
return origin_host in ["console.stage.redhat.com", "console.redhat.com"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this check enough to tell that the request is specifically from the UI? It looks to me like this header gets set to the target hostname for all HTTP requests

tests/helpers/mq_utils.py Outdated
@@ -129,6 +137,11 @@ def assert_delete_event_is_valid(

assert host.canonical_facts.get("insights_id") == event["insights_id"]

assert event["manual_delete"] == is_manual_delete
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
assert event["manual_delete"] == is_manual_delete
assert event["manual_delete"] is is_manual_delete

@FabriciaDinizRH FabriciaDinizRH marked this pull request as draft December 11, 2024 12:14
red-hat-konflux bot and others added 8 commits December 19, 2024 23:28
@red-hat-konflux @kruai @FabriciaDinizRH
chore(deps): update konflux references (RedHatInsights#2121)
75a8ba7 
Signed-off-by: red-hat-konflux <126015336+red-hat-konflux[bot]@users.noreply.github.com>
Co-authored-by: red-hat-konflux[bot] <126015336+red-hat-konflux[bot]@users.noreply.github.com>
Co-authored-by: Asa Price <[email protected]>
@dependabot @FabriciaDinizRH
build(deps-dev): bump pytest-subtests from 0.13.1 to 0.14.1 (RedHatIn…
0aa2f40 
…sights#2126)

Bumps [pytest-subtests](https://github.com/pytest-dev/pytest-subtests) from 0.13.1 to 0.14.1.
- [Release notes](https://github.com/pytest-dev/pytest-subtests/releases)
- [Changelog](https://github.com/pytest-dev/pytest-subtests/blob/main/CHANGELOG.rst)
- [Commits](pytest-dev/pytest-subtests@v0.13.1...v0.14.1)

---
updated-dependencies:
- dependency-name: pytest-subtests
  dependency-type: direct:development
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
@dependabot @kruai @FabriciaDinizRH
build(deps): bump boto3 from 1.35.77 to 1.35.78 (RedHatInsights#2128)
5b76d1a 
Bumps [boto3](https://github.com/boto/boto3) from 1.35.77 to 1.35.78.
- [Release notes](https://github.com/boto/boto3/releases)
- [Commits](boto/boto3@1.35.77...1.35.78)

---
updated-dependencies:
- dependency-name: boto3
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Asa Price <[email protected]>
@dependabot @FabriciaDinizRH
build(deps): bump boto3 from 1.35.78 to 1.35.79 (RedHatInsights#2130)
3461d39 
Bumps [boto3](https://github.com/boto/boto3) from 1.35.78 to 1.35.79.
- [Release notes](https://github.com/boto/boto3/releases)
- [Commits](boto/boto3@1.35.78...1.35.79)

---
updated-dependencies:
- dependency-name: boto3
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
@dependabot @FabriciaDinizRH
build(deps-dev): bump ruff from 0.8.2 to 0.8.3 (RedHatInsights#2131)
e20daa8 
Bumps [ruff](https://github.com/astral-sh/ruff) from 0.8.2 to 0.8.3.
- [Release notes](https://github.com/astral-sh/ruff/releases)
- [Changelog](https://github.com/astral-sh/ruff/blob/main/CHANGELOG.md)
- [Commits](astral-sh/ruff@0.8.2...0.8.3)

---
updated-dependencies:
- dependency-name: ruff
  dependency-type: direct:development
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
@jpramos123 @FabriciaDinizRH
feat(RHINENG-13314): Log specific SP fields defined in env config (Re…
bf0a15e 
…dHatInsights#2097)

* add env var to log system profile fields

* feat: implement logs to host methods

* test: implement tests

* feat: add system profile to expection logs

* feat: propagate log message

* add instructions to README.md
@red-hat-konflux @kruai @FabriciaDinizRH
chore(deps): update pre-commit hook astral-sh/ruff-pre-commit to v0.8…
f4303e4 
….3 (RedHatInsights#2129)

Signed-off-by: red-hat-konflux <126015336+red-hat-konflux[bot]@users.noreply.github.com>
Co-authored-by: red-hat-konflux[bot] <126015336+red-hat-konflux[bot]@users.noreply.github.com>
Co-authored-by: Asa Price <[email protected]>
@FabriciaDinizRH
add test
350baad
@FabriciaDinizRH FabriciaDinizRH marked this pull request as ready for review December 19, 2024 23:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thearifismail thearifismail Awaiting requested review from thearifismail

@kruai kruai Awaiting requested review from kruai

@jpramos123 jpramos123 Awaiting requested review from jpramos123

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@FabriciaDinizRH @kruai @jpramos123 @thearifismail