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

feat(*) move the GUI from :5683 to :5681/gui #915

Merged
merged 17 commits into from
Jul 27, 2020

Conversation

nickolaev
Copy link
Contributor

@nickolaev nickolaev commented Jul 17, 2020

Summary

Changes:

  • Stop listening at :5683
  • GUI is served at :5681/gui
  • When the GUI is disabled (e.g. in a remote mode), show a disabled page

Issues resolved

Fix #XXX

Documentation

Nikolay Nikolaev added 2 commits July 17, 2020 18:21
Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>
Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>
// Handle the GUI
container.Handle("/gui/", http.StripPrefix("/gui/", http.FileServer(resources.GuiDir)))
container.Handle("/api/", http.StripPrefix("/api/", container))
container.ServeMux.HandleFunc("/gui/config/", newApiServer.configHandler)
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think /api is needed anymore. You can just invoke root path of this server. I also think that /gui/config is obsolete, because all the information are in /config

Can you coordinate this with @bloqhead on GUI side? I can help if needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

I also suggest API on /, while the GUI on /gui.

Choose a reason for hiding this comment

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

I'm not opposed to using / for the API. I have to traverse up the URL path to access the API if the GUI is in /gui but that change is small. I think labeling them /api and /gui adds more clarity, but it's not crucial.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

Nikolay Nikolaev added 2 commits July 21, 2020 14:45
Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>
@nickolaev nickolaev force-pushed the feat/merge_api_and_gui_servers branch from 60517e5 to 6babf7e Compare July 21, 2020 11:45
Nikolay Nikolaev added 4 commits July 21, 2020 15:24
Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>
…gui_servers

Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>
Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>
…gui_servers

# Conflicts:
#	app/kumactl/pkg/install/k8s/control-plane/templates_vfsdata.go
Nikolay Nikolaev added 3 commits July 22, 2020 19:45
Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>
Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>
@nickolaev nickolaev marked this pull request as ready for review July 23, 2020 05:52
@nickolaev nickolaev requested a review from a team as a code owner July 23, 2020 05:52
Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>
@subnetmarco
Copy link
Contributor

Just want to confirm that API listens on / while the GUI on /gui. @nickolaev @bloqhead

}

Describe("enabled",
func() {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: we put it in the same line with describe

var stop chan struct{}
var baseUrl string

beforeEach := func(enabelGUI bool) {
Copy link
Contributor

Choose a reason for hiding this comment

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

how about setupServer instead of beforeEach

@@ -31,7 +35,8 @@ var (
)

type ApiServer struct {
server *http.Server
server *http.Server
GuiServerConfig *gui_server.GuiServerConfig
Copy link
Contributor

Choose a reason for hiding this comment

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

is it used?

Nikolay Nikolaev added 2 commits July 23, 2020 23:56
Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>
@nickolaev
Copy link
Contributor Author

Just want to confirm that API listens on / while the GUI on /gui. @nickolaev @bloqhead

Exactly, there are no other endpoints from the old GUI server.

Copy link
Contributor

@jakubdyszkiewicz jakubdyszkiewicz left a comment

Choose a reason for hiding this comment

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

Great!

@bloqhead
Copy link

Is there anything else we have left on this task? I have the changes in place in the GUI to align with this.

Nikolay Nikolaev added 2 commits July 25, 2020 08:27
@nickolaev nickolaev merged commit 6d67355 into master Jul 27, 2020
@nickolaev nickolaev deleted the feat/merge_api_and_gui_servers branch July 31, 2020 11:46
@bloqhead bloqhead mentioned this pull request Jul 31, 2020
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.

None yet

4 participants