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

fix: fixing consumer-group related dump config for deck. #159

Merged
merged 2 commits into from
Jan 14, 2025

Conversation

Prashansa-K
Copy link
Collaborator

@Prashansa-K Prashansa-K commented Jan 8, 2025

With changes added in go-kong, to skip
listing consumers while doing a GET call
on a consumer_group, dump config with gw
3.9+ starting showing issues. Associations
between a consumer and consumer-group were
missing. Thus, we are pushing the no listing
consumers feature behind a flag for now.

This change adds a new config that can be
set via a deck command flag. If set to true,
it will not list consumers-consumer group
associations.

Depends on Kong/go-kong#494

Related deck PR: Kong/deck#1486

Issues resolved

For Kong/deck#1483

Documentation

Testing

  • Unit tests
  • E2E tests
  • Manual testing on Universal
  • Manual testing on Kubernetes

@mheap
Copy link
Member

mheap commented Jan 8, 2025

@Prashansa-K This flag should apply to diff and sync too. It’s suitable when consumers are being managed in a different file.

By default, consumers should be included in all commands

@Prashansa-K
Copy link
Collaborator Author

@Prashansa-K This flag should apply to diff and sync too. It’s suitable when consumers are being managed in a different file.

By default, consumers should be included in all commands

Alright. That should be fine for this PR. We should not require any more code changes to support the said flag with sync and diff. I will update the description.

With changes added in go-kong, to skip
listing consumers while doing a GET call
on a consumer_group, dump config with gw
3.9+ starting showing issues. Associations
between a consumer and consumer-group were
missing. Thus, we are pushing the no listing
consumers feature behind a flag for now,
applicable only for dump command as a
starting point.

This change adds a new config that can be
set via a deck command flag. If set to true,
it will not list consumers-consumer group
associations in the dump file.
@Prashansa-K Prashansa-K force-pushed the fix/dump-cg-group-association branch from 88567a9 to b7beddd Compare January 13, 2025 13:04
@codecov-commenter
Copy link

Codecov Report

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

Project coverage is 28.61%. Comparing base (ae9c42b) to head (b7beddd).

Files with missing lines Patch % Lines
pkg/dump/dump.go 0.00% 36 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #159      +/-   ##
==========================================
- Coverage   28.67%   28.61%   -0.07%     
==========================================
  Files         106      106              
  Lines       15915    15950      +35     
==========================================
  Hits         4564     4564              
- Misses      10869    10904      +35     
  Partials      482      482              

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

@Prashansa-K Prashansa-K requested a review from GGabriele January 13, 2025 13:09
Copy link
Collaborator

@GGabriele GGabriele left a comment

Choose a reason for hiding this comment

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

LGTM.

Remember: Konnect API doesn't support the list_consumers query param, so the flag you are planning to add in decK is not compatible when running against Konnect

@Prashansa-K Prashansa-K merged commit c611fd4 into main Jan 14, 2025
19 checks passed
@Prashansa-K Prashansa-K deleted the fix/dump-cg-group-association branch January 14, 2025 11:19
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.

4 participants