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(crd): add KongConsumerGroup CRD #4325

Merged
merged 1 commit into from
Jul 19, 2023
Merged

feat(crd): add KongConsumerGroup CRD #4325

merged 1 commit into from
Jul 19, 2023

Conversation

programmer04
Copy link
Member

@programmer04 programmer04 commented Jul 12, 2023

What this PR does / why we need it:

The scope of this PR is to introduce a new CRD KongConsumerGroup and boilerplate controller for it that does nothing.

Which issue this PR fixes:

This is a part of #3728 - introducing support for Consumer Groups in KIC.

Special notes for your reviewer:

Touched files:

  • pkg/apis/configuration/v1beta1/kongconsumergroup_types.go
  • hack/generators/controllers/networking/main.go
  • internal/manager/controllerdef.go
  • config/crd/kustomization.yaml
  • PROJECT
  • CHANGELOG.md
  • test/envtest/crds_envtest_test.go

the rest was modified by code generators run with

make manifest && make generate

applying example configuration

apiVersion: configuration.konghq.com/v1beta1
kind: KongConsumerGroup
metadata:
  name: consumer-group-1
  annotations:
    kubernetes.io/ingress.class: kong

is successful in logs

ERRO[0007] Reconciler error.  error="cannot add unsupported kind \"configuration.konghq.com/v1beta1, Kind=KongConsumerGroup\" to the store" logger=controllers.KongConsumerGroup reconcileID="\"b059c577-e1ce-4189-b1e3-814b6cb34038\""

can be observed, because cache for this new Kind is implemented in a separate PR to make it easier for reviewer.

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 area/CRD Changes in existing CRDs or introduction of new ones labels Jul 12, 2023
@programmer04 programmer04 added this to the KIC v2.11.0 milestone Jul 12, 2023
@programmer04 programmer04 self-assigned this Jul 12, 2023
@codecov
Copy link

codecov bot commented Jul 12, 2023

Codecov Report

Patch coverage: 30.1% and project coverage change: -0.2 ⚠️

Comparison is base (09ae366) 65.5% compared to head (8597156) 65.3%.

Additional details and impacted files
@@           Coverage Diff           @@
##            main   #4325     +/-   ##
=======================================
- Coverage   65.5%   65.3%   -0.2%     
=======================================
  Files        154     154             
  Lines      17740   17866    +126     
=======================================
+ Hits       11632   11680     +48     
- Misses      5378    5456     +78     
  Partials     730     730             
Impacted Files Coverage Δ
...trollers/configuration/zz_generated_controllers.go 26.2% <16.9%> (-1.1%) ⬇️
internal/manager/controllerdef.go 98.8% <100.0%> (+<0.1%) ⬆️

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@programmer04 programmer04 added the do not merge let the author merge this, don't merge for them. label Jul 12, 2023
@programmer04 programmer04 force-pushed the add-consumergroup branch 4 times, most recently from 4530afb to a68adba Compare July 17, 2023 11:54
@programmer04 programmer04 marked this pull request as ready for review July 17, 2023 11:54
@programmer04 programmer04 requested a review from a team as a code owner July 17, 2023 11:54
@programmer04 programmer04 removed the do not merge let the author merge this, don't merge for them. label Jul 17, 2023
@programmer04 programmer04 enabled auto-merge (squash) July 17, 2023 11:55
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.

Are we able to add the store code in here as well? Those functions are essentially boilerplate AFAIK (they probably could also be templated even, we just never bothered). I'd prefer to have the intermediate PR be "does effectively nothing, successfully" versus "does nothing, expected to produce benign errors".

Stuffing both the scaffolding and actual functional code into the same PR as separate commits would also be fine. The Github UX for per-commit diffs within a PR isn't perfect, but it is available. I at least can glaze over the parts I know are boilerplate since it's familiar boilerplate. Dunno what overall preference would be--it'd be a good retro item.

@programmer04 programmer04 force-pushed the add-consumergroup branch 2 times, most recently from e6ce590 to ce5a14a Compare July 18, 2023 07:20
@programmer04
Copy link
Member Author

From my perspective, it doesn't matter much @rainest. I can do whatever is preferred. Somewhere the division has to be made to keep a reasonable size of PRs. I consider generating messages in case of try of usage as a thing that clearly indicates the WIP state of this. We do not have an approach for testing put into the cache. The next PR will include both cache and parser with more e2e-like tests

@programmer04 programmer04 added do not merge let the author merge this, don't merge for them. and removed do not merge let the author merge this, don't merge for them. labels Jul 18, 2023
@programmer04 programmer04 merged commit e5969be into main Jul 19, 2023
36 of 37 checks passed
@programmer04 programmer04 deleted the add-consumergroup branch July 19, 2023 21:14
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

4 participants