-
Notifications
You must be signed in to change notification settings - Fork 129
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: populate missing 'enabled' service field from file #677
Conversation
Codecov Report
@@ Coverage Diff @@
## main #677 +/- ##
==========================================
+ Coverage 44.31% 44.32% +0.01%
==========================================
Files 74 74
Lines 8790 8792 +2
==========================================
+ Hits 3895 3897 +2
Misses 4532 4532
Partials 363 363
Continue to review full report at Codecov.
|
69897a5
to
d225235
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For reference, Kong/kong@1339494
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code 👍, can you add a pending release changelog entry for this?
Please add a test. |
Without this decK is failing at parsing the 'enabled' field for services entities.
d225235
to
9d3d39a
Compare
I still need to get used to that :( added now
done |
file/reader_test.go
Outdated
assert.Equal(kong.Target{ | ||
Target: kong.String("198.51.100.11:80"), | ||
}, | ||
c.Upstreams[0].Targets[0].Target) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll recommend writing a targetted test that reads a file which only has a single service and doing appropriate assertions.
I also recommend having a sub-test t.Run("enabled field for service is read", ...
under the test. That provides:
- a description of what is being tested
- a narrow unit test that can be read and fixed later on easily
- ability for future sub-tests to be added here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sounds good
9d3d39a
to
76ff616
Compare
76ff616
to
ab9ef4a
Compare
Without this decK is failing at parsing the 'enabled' field
for services entities.