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

refactor: decouple toDeckConfig/OnUpdate from controller, and expose as libraries #1029

Merged
merged 12 commits into from
Feb 10, 2021

Conversation

mflendrich
Copy link
Contributor

This PR:

  • introduces the package pkg/deckgen that exposes ToDeckContent() acting as a converter from KongState to file.Content
    • introduces dependency injection for auxiliary info required by ToDeckContent() (as opposed to ToDeckContent() querying data sources for info directly)
  • introduces the package pkg/sendconfig that connects to Kong Admin API in order to configure it according to file.Content
  • straightens out the logic of SHA diffing

Why we need this change: in order to expose the existing (KongState -> file.Content and file.Content -> Kong Admin API) operations as a library, to be used by the prototype port of KIC to kubebuilder.

Please review commit-by-commit. None of the commits (except for the modified SHA diffing) should change existing behavior. I recommend merging without squashing either - because individual moves are much easier to comprehend in individual commits.

Testing plan:

  • Unit tests ran, coverage not changed here. Writing unit tests (for the now exposed functions that are now much easier to unit test) out of scope of this PR.
  • Tested the DB-less mode to the extent of the present E2E test.
  • DB mode not tested automatically. In parallel with the review, the author will perform the test manually (the test will include manual validation of the SHA logic).

@codecov
Copy link

codecov bot commented Feb 4, 2021

Codecov Report

Merging #1029 (90f818d) into next (b3be780) will increase coverage by 2.41%.
The diff coverage is 16.25%.

Impacted file tree graph

@@            Coverage Diff             @@
##             next    #1029      +/-   ##
==========================================
+ Coverage   51.42%   53.84%   +2.41%     
==========================================
  Files          33       33              
  Lines        3327     3172     -155     
==========================================
- Hits         1711     1708       -3     
+ Misses       1485     1332     -153     
- Partials      131      132       +1     
Impacted Files Coverage Δ
cli/ingress-controller/main.go 0.23% <0.00%> (ø)
pkg/deckgen/generate.go 0.00% <0.00%> (ø)
pkg/util/plugin_schema_helper.go 0.00% <0.00%> (ø)
pkg/sendconfig/sendconfig.go 20.00% <20.00%> (ø)
pkg/deckgen/deckgen.go 39.18% <39.18%> (ø)
pkg/parser/parser.go 83.92% <0.00%> (-1.34%) ⬇️
internal/ingress/controller/event_handler.go
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b3be780...90f818d. Read the comment docs.

@mflendrich mflendrich changed the base branch from main to next February 4, 2021 20:24
rainest
rainest previously approved these changes Feb 8, 2021
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.

I'll need to rework #991 around this but have had difficulty fitting it around the existing monolithic functions, so agreed that breaking this functionality out will make it easier to add new components around them. Thanks for the refactor 👍

shaneutt
shaneutt previously approved these changes Feb 9, 2021
Copy link
Contributor

@shaneutt shaneutt left a comment

Choose a reason for hiding this comment

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

Looks good. I have no blocking comments, but I do have a couple smaller things related to godoc and comments. 👍

pkg/deckgen/deckgen.go Show resolved Hide resolved
pkg/deckgen/deckgen.go Show resolved Hide resolved
pkg/deckgen/deckgen.go Show resolved Hide resolved
pkg/deckgen/deckgen.go Show resolved Hide resolved
pkg/deckgen/deckgen.go Show resolved Hide resolved
pkg/deckgen/generate.go Show resolved Hide resolved
pkg/sendconfig/sendconfig.go Show resolved Hide resolved
pkg/sendconfig/sendconfig.go Outdated Show resolved Hide resolved
Comment on lines +10 to +11
// FIXME
// decK will release this official API soon, use that and remove this code.
Copy link
Contributor

Choose a reason for hiding this comment

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

I know this was just imported from existing code, but can we improve this with an issue link or any other breadcrumbs for the reader?

The clean way to enable SetDebugMode would be to pass a client with
SetDebugMode enabled from main().
@mflendrich mflendrich dismissed stale reviews from shaneutt and rainest via 90f818d February 10, 2021 11:20
@mflendrich mflendrich merged commit 90f818d into next Feb 10, 2021
@mflendrich mflendrich deleted the refactor/breakup-controller branch February 10, 2021 15:14
rainest pushed a commit that referenced this pull request Feb 11, 2021
Re-add config dump following merger of decouple refactor (PR #1029).

Auto-merge created a confusing conflict, so it was easier to merge it in
with strategy "theirs" and then place the dump path in OnUpdate and
helper back after the fact.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants