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

feat: remove deprecated kafka_storage_size #1421

Merged
merged 1 commit into from
Feb 15, 2023
Merged

feat: remove deprecated kafka_storage_size #1421

merged 1 commit into from
Feb 15, 2023

Conversation

valeriiashapoval
Copy link
Contributor

Description

(https://issues.redhat.com/browse/MGDSTRM-8970)

Checklist (Definition of Done)

  • All acceptance criteria specified in JIRA have been completed
  • Unit and integration tests added that prove the fix is effective or the feature works (tested against emulated and non-emulated OCM environment)
  • Documentation added for the feature
  • CI and all relevant tests are passing
  • Code Review completed
  • Verified independently by reviewer
  • All PR comments are resolved either by addressing them or creating follow up tasks
  • Required metrics/dashboards/alerts have been added (or PR created).
  • Required Standard Operating Procedure (SOP) is added.
  • JIRA has been created for changes required on the client side

@valeriiashapoval valeriiashapoval requested a review from a team as a code owner November 30, 2022 09:57
@codecov
Copy link

codecov bot commented Nov 30, 2022 •
edited

Codecov Report

Merging #1421 (2b86e31) into main (5e62613) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1421      +/-   ##
==========================================
- Coverage   82.76%   82.75%   -0.01%     
==========================================
  Files         154      154              
  Lines       14196    14190       -6     
==========================================
- Hits        11749    11743       -6     
  Misses       2025     2025              
  Partials      422      422
Flag Coverage Δ
unittests 82.75% <100.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
internal/kafka/internal/handlers/admin_kafka.go 94.50% <100.00%> (-0.06%) ⬇️
internal/kafka/internal/handlers/validation.go 89.31% <100.00%> (-0.06%) ⬇️
internal/kafka/internal/presenters/admin_kafka.go 85.93% <100.00%> (-0.22%) ⬇️
internal/kafka/internal/presenters/kafka.go 99.11% <100.00%> (-0.01%) ⬇️
internal/kafka/internal/services/kafka.go 77.39% <100.00%> (ø)

@miguelsorianod
Copy link
Contributor

Any relevant SOPs should be updated too

@valeriiashapoval valeriiashapoval force-pushed the remove-deprecated-fields branch 3 times, most recently from 8f50172 to 9f24915 Compare December 8, 2022 14:46
@pawelpaszki
Copy link
Contributor

@machi1990 - can you confirm that the db migration shouldn't be a breaking change, since we used MaxDataRetentionSize in the presenter anyway?

@machi1990
Copy link
Contributor

@machi1990 - can you confirm that the db migration shouldn't be a breaking change, since we used MaxDataRetentionSize in the presenter anyway?

No it shouldn't be a breaking change but just for awareness this change might cause temporarily 5xx errors from old pods when a request is done through them to access the kafka requests table. It could be solved by doing a two phased migration but it might not be worth it given the complexity.
What we've been doing thus far is to be aware of the issue occurring. Be on alert / watch out for any monitoring alerts e.g in sentry, until they clear because service should resume once the new pods are rolled out.

@valeriiashapoval valeriiashapoval force-pushed the remove-deprecated-fields branch 2 times, most recently from b74c2e6 to 1a5da1f Compare December 15, 2022 13:59
@machi1990
Copy link
Contributor

What's the status of this PR? @valeriiashapoval

@machi1990 machi1990 added the needs-rebase Added to PR that requires a rebase (likely because of a conflict) before they can be merged label Feb 14, 2023
@valeriiashapoval
Copy link
Contributor Author

What's the status of this PR? @valeriiashapoval

thanks @machi1990 for reminding about this one, just rebased the branch

@machi1990 machi1990 removed the needs-rebase Added to PR that requires a rebase (likely because of a conflict) before they can be merged label Feb 14, 2023
pawelpaszki
pawelpaszki approved these changes Feb 15, 2023
View reviewed changes
Copy link
Contributor

@pawelpaszki pawelpaszki left a comment

Choose a reason for hiding this comment

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

LGTM

@pawelpaszki pawelpaszki merged commit b66bea2 into bf2fc6cc711aee1a0c2a:main Feb 15, 2023
@valeriiashapoval valeriiashapoval deleted the remove-deprecated-fields branch February 15, 2023 10:06
machi1990
machi1990 reviewed Feb 15, 2023
View reviewed changes
openapi/kas-fleet-manager.yaml
@@ -1409,10 +1409,6 @@ components:
description: "This field is now deprecated, please use the /api/kafkas_mgmt/v1/instance_types/{cloud_provider}/{cloud_region} endpoint to retrieve the field instead."
reauthentication_enabled:
type: boolean
kafka_storage_size:
Copy link
Contributor

Choose a reason for hiding this comment

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

Would have been good to bump the API version as well over here https://github.com/valeriiashapoval/kas-fleet-manager/blob/2b86e31a2fcea4b28dc7f0bd4e3708f38d3d558b/openapi/kas-fleet-manager.yaml#L5.

FYI @valeriiashapoval

machi1990
machi1990 reviewed Feb 15, 2023
View reviewed changes
openapi/kas-fleet-manager-private-admin.yaml
@@ -312,10 +308,6 @@ components:
type: string
kafka_ibp_version:
type: string
kafka_storage_size:
Copy link
Contributor

Choose a reason for hiding this comment

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

We could consider bumping the API version https://github.com/valeriiashapoval/kas-fleet-manager/blob/2b86e31a2fcea4b28dc7f0bd4e3708f38d3d558b/openapi/kas-fleet-manager-private-admin.yaml#L5

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@machi1990 machi1990 machi1990 left review comments

@pawelpaszki pawelpaszki pawelpaszki approved these changes

@miguelsorianod miguelsorianod Awaiting requested review from miguelsorianod

Assignees
No one assigned
Labels
api-change kafka
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@valeriiashapoval @miguelsorianod @pawelpaszki @machi1990