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

feat: adding consumer-group policy overrides support in deck #1518

Merged
merged 6 commits into from
Feb 7, 2025

Conversation

Prashansa-K
Copy link
Contributor

@Prashansa-K Prashansa-K commented Feb 5, 2025

@Prashansa-K Prashansa-K changed the title feat: ading consumer-group policy overrides support in deck feat: adding consumer-group policy overrides support in deck Feb 5, 2025
@codecov-commenter
Copy link

codecov-commenter commented Feb 6, 2025

Codecov Report

Attention: Patch coverage is 0% with 43 lines in your changes missing coverage. Please review.

Project coverage is 29.09%. Comparing base (107c160) to head (39e0e77).

Files with missing lines Patch % Lines
cmd/gateway_dump.go 0.00% 18 Missing ⚠️
cmd/common.go 0.00% 10 Missing ⚠️
tests/integration/test_utils.go 0.00% 7 Missing ⚠️
cmd/gateway_diff.go 0.00% 4 Missing ⚠️
cmd/gateway_sync.go 0.00% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1518      +/-   ##
==========================================
- Coverage   29.28%   29.09%   -0.19%     
==========================================
  Files          62       62              
  Lines        6653     6682      +29     
==========================================
- Hits         1948     1944       -4     
- Misses       4567     4599      +32     
- Partials      138      139       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Prashansa-K
Copy link
Contributor Author

Prashansa-K commented Feb 6, 2025

@harshadixit12 I have added the fixes to Konnect integration test in this separate PR: #1519, so that this PR doesn't become massive.
Since we retrieve kong version now (refer: Kong/go-database-reconciler#195), Konnect related tests had to be separated out for testing.

@@ -1925,6 +1925,11 @@ const complexQueryForDegraphqlRoute = `query SearchPosts($filters: PostsFilters)
}
`

const errorConsumerGroupPolicies = "a rate-limiting-advanced plugin with config.consumer_groups\n" +
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we capitalise the first letter of error message?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This error comes from GDR. It is copied as it is. https://github.com/Kong/go-database-reconciler/blob/main/pkg/utils/utils.go#L29
Also, errors in go are not capitalised by convention. You get lint warnings for the same.

@harshadixit12
Copy link
Contributor

@Prashansa-K had a few questions -

  1. Why do we have separate tests for gateway versions 3.8 and 3.9?
  2. Do we need tests for the diff command?

@Prashansa-K
Copy link
Contributor Author

@Prashansa-K had a few questions -

  1. Why do we have separate tests for gateway versions 3.8 and 3.9?
  2. Do we need tests for the diff command?
  1. 3.9 introduces some schema changes in the RLA plugin. So, different files and expected state need to be maintained for both these versions.
  2. Not required. It is a dry-run version of sync and we have covered sync tests.

…not fail (#1519)

* tests: separated Konnect tests to ensure kong version retrieval does
not fail

* tests: fixed test for dql routes in konnect

* tests: fixed test condition

* tests: fixed deck token setting position
@Prashansa-K Prashansa-K merged commit 5f266f9 into main Feb 7, 2025
23 checks passed
@Prashansa-K Prashansa-K deleted the feat/cg-policy-overrides branch February 7, 2025 04:30
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