-
Notifications
You must be signed in to change notification settings - Fork 390
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
(TASK #1) Adding Service Functionality #167
Conversation
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.
Thanks for this PR, it's awesome, especially since you took time to write tests.
I just have a few minor comments. I'll merge this PR when they are addressed.
src/html/services/form.html
Outdated
<div class="card-panel teal lighten-2"> | ||
<h5 class="white-text"> | ||
<i class="material-icons small">highlight</i> | ||
What's an Service? |
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.
Typo: "What's a Service?"
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.
Updated
src/html/services/index.html
Outdated
<table class="bordered" ng-show="total > 0"> | ||
<thead> | ||
<tr> | ||
<th ng-repeat="prop in env.schemas.service.properties">{{prop.display_name}}</th> |
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 wouldn't display all the information of a Service on this page. This leads to many columns and isn't very readable.
This is what it looks like in my browser:
I would collapse "protocol", "host", "port" and "path" into the single column named "url" (which is empty by the way), and not display "updated at", "created at", and the various options (retry, timeouts...). The user can still click edit to see the details of a given Service.
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.
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.
Looks good to me
src/html/index.html
Outdated
<a href="#!/services" class="waves-effect"> | ||
<i class="material-icons">dvr</i> Services | ||
</a> | ||
</li> | ||
<li> | ||
<a href="#!/apis" class="waves-effect"> | ||
<i class="material-icons">apps</i> APIs |
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.
If env.schemas.service
exists, I think you can add "(deprecated)" after "APIs".
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.
Done.
|
||
describe('Service Creation testing', () => { | ||
// Only run tests if the Kong version is 0.13 as Services are supported in version 0.13 only | ||
if (process.env.KONG_VERSION === '0.13') { |
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.
Here you'll want to check if Kong version is 0.13 or higher.
You can use semver for that:
var semver = require('semver');
if (semver.lt(process.env.KONG_VERSION, '0.13.0')) {
// skip tests
} else {
// run tests
}
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.
Nice suggestion.
However, I am not able to use semver to compare the versions. I am getting the following error if I use semver in tests:
Service Creation testing encountered a declaration exception
Message:
TypeError: Invalid Version: 0.12
Stack:
TypeError: Invalid Version: 0.12
at new SemVer (/Users/pkumar/kong-dashboard/node_modules/semver/semver.js:305:11)
at compare (/Users/pkumar/kong-dashboard/node_modules/semver/semver.js:578:10)
at Function.lt (/Users/pkumar/kong-dashboard/node_modules/semver/semver.js:612:10)
at Suite.fdescribe (/Users/pkumar/kong-dashboard/tests/cases/services/create-service.spec.js:16:16)
at Object.<anonymous> (/Users/pkumar/kong-dashboard/tests/cases/services/create-service.spec.js:14:1)
at Module._compile (internal/modules/cjs/loader.js:702:30)
at Object.Module._extensions..js (internal/modules/cjs/loader.js:713:10)
I need to spend more time figuring this issue out. Looks like by node (v10.2.1) and npm (5.6.0) versions are not liking Semver. Let me know if this is a blocker for merging this pull request. I will be working on route integration next and will revisit this issue once I write tests for routes.
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.
Ok no worry I'll take care of that.
Looks like about half of the builds are failing due to the following error:
I think this is an intermittent issue as few builds succeeded and my code change is not at all significant. @PGBI Since I don't have necessary permissions, would you mind restarting the builds and see if this issue persist? |
yep, seems like Travis had a temporary issue. I restarted the tests, we'll see how it goes. Next time, you can also restart the tests by just pushing an empty commit. |
@PGBI Thanks for restarting the tests. All the tests passed this time. Next time I will push an empty commit if needed. |
This is the first commit in the series of commits for adding Kong 0.13.X support.
This change adds the functionality for creating, listing, deleting and updating the Kong Services
Next Steps:
Task #2 - Adding Routes Functionality
Task #3 - Plugins integration for Services and Routes