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: Consumer Groups can be configured in Consumer #4419

Merged
merged 1 commit into from
Jul 28, 2023

Conversation

programmer04
Copy link
Member

@programmer04 programmer04 commented Jul 27, 2023

What this PR does / why we need it:

Introduce the optional field ConsumerGroups to KongConsumer CRD that allows configuring Consumer Groups for Consumer. Add implementation and enhance integration test to cover it.

Which issue this PR fixes:

part of #3728

Special notes for your reviewer:

Integration test TestConsumerGroup is executed for two set-ups:

  • DBMode (Postgres) - it passes without problems 🟢
  • DBLess - it doesn't pass due to a bug in Kong Gateway reported by me to them in FTI-5264 (thus the status of this PR is blocked) 🔴 so test is skipped for such configuration for now

The golden test has been introduced for consumer groups with consumers.

PR Readiness Checklist:

Complete these before marking the PR as ready to review:

  • the CHANGELOG.md release notes have been updated to reflect any significant (and particularly user-facing) changes introduced by this PR

@programmer04 programmer04 added area/feature New feature or request blocked area/CRD Changes in existing CRDs or introduction of new ones labels Jul 27, 2023
@programmer04 programmer04 self-assigned this Jul 27, 2023
@programmer04 programmer04 force-pushed the add-consumergroup-field-consumer branch from f745b8d to 3eeea9e Compare July 27, 2023 13:35
@programmer04 programmer04 changed the title feat(crd): add ConsumerGroups field in KongConsumer feat: Consumer Groups can be configured in Consumer Jul 27, 2023
@programmer04 programmer04 force-pushed the add-consumergroup-field-consumer branch from 3eeea9e to 6b240b3 Compare July 27, 2023 13:40
@programmer04 programmer04 added this to the KIC v2.11.0 milestone Jul 27, 2023
@programmer04 programmer04 marked this pull request as ready for review July 27, 2023 13:44
@programmer04 programmer04 requested a review from a team as a code owner July 27, 2023 13:44
@codecov
Copy link

codecov bot commented Jul 27, 2023

Codecov Report

Patch coverage: 7.6% and project coverage change: -5.8% ⚠️

Comparison is base (1d7296b) 67.5% compared to head (631da10) 61.7%.

❗ Current head 631da10 differs from pull request most recent head 75d72d4. Consider uploading reports for the commit 75d72d4 to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##            main   #4419     +/-   ##
=======================================
- Coverage   67.5%   61.7%   -5.8%     
=======================================
  Files        159     159             
  Lines      18772   18762     -10     
=======================================
- Hits       12681   11588   -1093     
- Misses      5327    6502   +1175     
+ Partials     764     672     -92     
Files Changed Coverage Δ
internal/dataplane/deckgen/generate.go 43.5% <0.0%> (-14.7%) ⬇️
internal/dataplane/kongstate/consumer.go 100.0% <ø> (ø)
internal/dataplane/kongstate/kongstate.go 64.5% <11.1%> (-7.1%) ⬇️

... and 33 files with indirect coverage changes

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

Copy link
Contributor

@rainest rainest left a comment

Choose a reason for hiding this comment

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

Code looks fine on our end. Some copyediting comments.

We may want to dispense with the integration test (especially since it's broken for reasons beyond our control) and only do unit or golden tests for the consumer+group assignment. Ultimately (once plugin assignment is complete) we should end up with a single test that confirms proper mapping by assigning a plugin to the consumer group and confirming that it runs.

Querying the admin API to confirm the objects is atypical.(I thought it odd there was a special workspaced client if case, and then realized that no other integration test has this because this is the only one that checks the admin API. KIC integration tests generally observe the effects of their configuration instead of the config itself, since the effect implies the presence of the configuration.

internal/dataplane/kongstate/kongstate.go Outdated Show resolved Hide resolved
pkg/apis/configuration/v1/kongconsumer_types.go Outdated Show resolved Hide resolved
@programmer04
Copy link
Member Author

programmer04 commented Jul 28, 2023

In terms of testing @rainest I consider this state as intermediate as I've written in PR that introduces this test #4387

The introduced integration test TestConsumerGroup will evolve in the future to cover all features of it e.g. adding Consumers to Consumer Group. Since Consumer Group doesn't do anything useful yet for now check directly in Gateway if it was created. Later it will be replaced with a proper testing scenario.

So it's a pretty low cost to have it like that for now and later when the full feature will be implemented adjust it. It allowed me to discover problems during development like the broken Kong Gateway.

I've introduced a golden test too.

@programmer04 programmer04 force-pushed the add-consumergroup-field-consumer branch from 6b240b3 to 631da10 Compare July 28, 2023 07:44
@programmer04 programmer04 force-pushed the add-consumergroup-field-consumer branch 3 times, most recently from eeb8ad6 to 86ab16b Compare July 28, 2023 13:34
@programmer04 programmer04 force-pushed the add-consumergroup-field-consumer branch from 86ab16b to 75d72d4 Compare July 28, 2023 14:41
@programmer04 programmer04 enabled auto-merge (squash) July 28, 2023 15:20
@programmer04 programmer04 merged commit a6e5a0d into main Jul 28, 2023
31 checks passed
@programmer04 programmer04 deleted the add-consumergroup-field-consumer branch July 28, 2023 19:31
@rainest rainest mentioned this pull request Jul 28, 2023
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/CRD Changes in existing CRDs or introduction of new ones area/feature New feature or request size/L
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants