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.

Sign up for GitHub

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

Jump to bottom

added serveral endpoints #28

Merged
merged 10 commits into from
Apr 5, 2022
Merged

added serveral endpoints #28

merged 10 commits into from
Apr 5, 2022

Conversation

jasmingacic
Copy link
Contributor

@jasmingacic
added serveral endpoints
5946f13 
Signed-off-by: Jasmin Gacic <jasmin.gacic@gmail.com>
@jasmingacic
reverted dockerfile
d77d3e3 
Signed-off-by: Jasmin Gacic <jasmin.gacic@gmail.com>
@jasmingacic
various fixes
216664d 
Signed-off-by: Jasmin Gacic <jasmin.gacic@gmail.com>
@jasmingacic
fixed test file
1d581ce 
Signed-off-by: Jasmin Gacic <jasmin.gacic@gmail.com>
@jasmingacic
fixed tests some more
3fc7efa 
Signed-off-by: Jasmin Gacic <jasmin.gacic@gmail.com>
@jasmingacic
fixed GH actions
14e745c 
Signed-off-by: Jasmin Gacic <jasmin.gacic@gmail.com>
@jasmingacic
added ports to serviceitem
eea2243 
Signed-off-by: Jasmin Gacic <jasmin.gacic@gmail.com>
@jasmingacic
extended getFleets
f46c25f 
Signed-off-by: Jasmin Gacic <jasmin.gacic@gmail.com>
@topliceanurazvan
Copy link
Member

topliceanurazvan commented Apr 5, 2022 •
edited
Loading

Looks good @jasmingacic, all the endpoints seems to be in place. The fleets CRD endpoint is missing from description, but I've tested it on the server and it looks like it's working.

kylehodgetts
kylehodgetts suggested changes Apr 5, 2022
View reviewed changes
Copy link
Contributor

@kylehodgetts kylehodgetts left a comment

Choose a reason for hiding this comment

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

Some small changes. Could you also go fmt all of the code, please? 😁

Makefile Outdated
@@ -9,3 +9,5 @@ run:

run-minikube:
docker-compose -f docker-compose.yaml -f docker-compose-minikube.yaml up --build -d
test:
cd ./server && FAKE=true go test -v ./...
Copy link
Contributor

Choose a reason for hiding this comment

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

newline

server/.openapi-generator-ignore Outdated
go/Dockerfile
Dockerfile
Copy link
Contributor

Choose a reason for hiding this comment

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

newline

server/README.md
@@ -25,20 +25,10 @@ go run main.go

To run the server in a docker container
```
make build
docker build --network=host -t openapi .
Copy link
Contributor

Choose a reason for hiding this comment

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

can you put make build back, please?

Copy link
Contributor

Choose a reason for hiding this comment

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

and the other make commands too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess I was not aware of this. change :D
I will have it reverted

server/go/api.go Outdated
// FleetsApiRouter defines the required methods for binding the api requests to a responses for the FleetsApi
// The FleetsApiRouter implementation should parse necessary information from the http request,
// pass the data to a FleetsApiServicer to perform the required actions, then write the service results to the http response.
type FleetsApiRouter interface {
type FleetsApiRouter interface {
Copy link
Contributor

Choose a reason for hiding this comment

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

Has this file been go fmt'ed?

server/go/api.go
GetEnvoyFleet(http.ResponseWriter, *http.Request)
GetEnvoyFleetCRD(http.ResponseWriter, *http.Request)
Copy link
Contributor

Choose a reason for hiding this comment

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

This returns the CRD definition?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, the object as is

server/go/api_services_service_impl.go
@@ -14,26 +14,43 @@ import (
"net/http"
"strings"

kusk "github.com/GIT_USER_ID/GIT_REPO_ID/kusk"
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this need an alias if the package is already kusk?

server/go/api_services_service_impl.go
// GetService - Get details for a single service
func (s *ServicesApiService) GetService(ctx context.Context, namespace string, name string) (ImplResponse, error) {
svc, err := s.kuskClient.GetSvc(namespace, name)
if err != nil {
return Response(http.StatusInternalServerError, err), err
}

port := ServicePortItem{
Copy link
Contributor

Choose a reason for hiding this comment

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

go fmt

server/go/api_static_routes_service_impl.go

// GetStaticRouteCRD - Get static route CRD
func (s *StaticRoutesApiService) GetStaticRouteCRD(ctx context.Context, namespace string, name string) (ImplResponse, error) {
staticRoute, err := s.kuskClient.GetStaticRoute(namespace, name)
Copy link
Contributor

Choose a reason for hiding this comment

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

staticRoute, err := s.kuskClient.GetStaticRoute(namespace, name)

Should this be called GetStaticRouteCRD(...) ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not really. The only difference is that here we don't do any operations on fleet object we just return it at the end of the method.

Copy link
Contributor

Choose a reason for hiding this comment

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

Im not sure I understand, the Service method is called GetStaticRouteCRD but inside we call the kuskClient GetStaticRoute? that's the correct behaviour?

server/go/routers.go Outdated
@@ -216,4 +216,4 @@ func parseInt32ArrayParameter(param, delim string, required bool) ([]int32, erro
}

return ints, nil
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

newline

@jasmingacic
addressing remarks from the PR
7bb9cbb 
Signed-off-by: Jasmin Gacic <jasmin.gacic@gmail.com>
@jasmingacic
formatting
3123b62 
Signed-off-by: Jasmin Gacic <jasmin.gacic@gmail.com>
@jasmingacic jasmingacic merged commit eceb80e into main Apr 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@topliceanurazvan topliceanurazvan topliceanurazvan approved these changes

@kylehodgetts kylehodgetts kylehodgetts approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@jasmingacic @topliceanurazvan @kylehodgetts