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

Adding support for Kong 0.12. Upgrading version to 3.1.0 #149

Merged
merged 2 commits into from
Jan 28, 2018

Conversation

peeyush1234
Copy link
Contributor

Hi @PGBI ,

As per the discussion #148, I have created a new pull request to add support for Kong 0.12.

Please review and let me know if this change can be merged.

I ran all the tests and they passed.

Best,
Peeyush

Copy link
Owner

@PGBI PGBI left a comment

Choose a reason for hiding this comment

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

This looks good! Thanks!

I just have two small comments, I'll wait for them to be addressed before merging.

README.md Outdated
|-----------|------------------------------|--------------|
| 1.x.x | 0.6.x, 0.7.x, 0.8.x, 0.9.x | |
| 2.x.x | 0.10.x | |
| 3.x.x | 0.9.x, 0.10.x, 0.11.x, 0.12.x| >= 6.0.0 |
Copy link
Owner

Choose a reason for hiding this comment

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

3.0.x should have it's own row because it will never be compatible with Kong 0.12.

| 3.0.x  | 0.9.x, 0.10.x, 0.11.x         | >= 6.0.0 |
| 3.x.x  | 0.9.x, 0.10.x, 0.11.x, 0.12.x | >= 6.0.0 |

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay.

// For Kong versions other than 0.12
if ($scope.active) {
page += "/active";
}
Copy link
Owner

Choose a reason for hiding this comment

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

Problem with this logic is that when a new minor version of Kong will be available, kong dashboard will use the /active endpoint back again. I doubt that Kong 0.13 will go back to Kong 0.11 behavior for this endpoint.

How about this kind of logic?

if (["0.9.", "0.10", "0.11"].includes(env.kong_version.substring(0,4)) {
    page += $scope.active ? "/active" : "";
} else {
    page += !$scope.active ? "/all" : "";
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay. Make sense.

@peeyush1234
Copy link
Contributor Author

@PGBI I have updated the code based on your feedback.

@PGBI PGBI merged commit 8a0d047 into PGBI:3.0 Jan 28, 2018
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.

2 participants