konveyor / tackle-pathfinder

Tackle Pathfinder application
Apache License 2.0
16 stars 23 forks source link

TACKLE-132 Indentified Risks #46

Closed jonathanvila closed 3 years ago

jonathanvila commented 3 years ago

Issue : https://github.com/konveyor/tackle-pathfinder/issues/47

Features covered

Unit , Integration and End2End Tests cases covered

Pre steps Minikube:

  1. Install minikube : https://minikube.sigs.k8s.io/docs/start/
  2. Start minikube : minikube start --kubernetes-version=v1.20.2 --cpus 4 --memory 12000
  3. Enable ingress addons : minikube addons enable ingress
  4. Create tackle namespace
    kubectl create namespace tackle
  5. Deploy tackle ( to have the keycloak instance running ) on your K8s cluster
    $ kubectl apply -f  https://raw.githubusercontent.com/konveyor/tackle/main/kubernetes/kubernetes-tackle.yaml -n tackle
  6. Edit the Ingress object to allow direct request to Pathfinder API
    $ kubectl edit ingress tackle -n tackle

    Add this below paths:

     - backend:
          service:
            name: tackle-pathfinder
            port:
              number: 8080 
        path: /pathfinder
        pathType: ImplementationSpecific
  7. Build and push
    $ ./mvnw -U -B package -Dquarkus.container-image.push=true -Dquarkus.container-image.group={your quay user} -Dquarkus.container-image.registry=quay.io -Dquarkus.container-image.username={your quay user} -Dquarkus.container-image.password={your quay pwd} -Pnative
  8. Change the image in the deployment tackle-pathfinder to be the one you have pushed
  9. Rollout to update the image
    $ kubectl rollout restart deployment tackle-pathfinder -n tackle

Test case with Minikube : Do the full API test

$ .github/scripts/check_api.sh

Expect

+++++ API CHECK SUCCESSFUL ++++++

Pre steps non containerised local test:

  1. run keycloak
    podman run -it --name keycloak --rm \     
            -e KEYCLOAK_USER=admin -e KEYCLOAK_PASSWORD=admin -e KEYCLOAK_IMPORT=/tmp/keycloak/quarkus-realm.json \
            -e DB_VENDOR=h2 -p 8180:8080 -p 8543:8443 -v ./src/main/resources/keycloak:/tmp/keycloak:Z \
            jboss/keycloak:12.0.2
  2. run postgresql
    podman run -it \                          
            --name postgres-pathfinder -e POSTGRES_USER=pathfinder \                                               
            -e POSTGRES_PASSWORD=pathfinder -e POSTGRES_DB=pathfinder_db \                              
            -p 5433:5432 postgres:10.6
  3. run application
    ./mvnw quarkus:dev

Test case with local non containerised : Do the full API test

$ .github/scripts/check_api.sh localhost:8085 localhost:8180

Expect

+++++ API CHECK SUCCESSFUL ++++++
jonathanvila commented 3 years ago

@jonathanvila Thanks for this PR, here are some questions I'd like to be addressed:

  • The response of GET /assessments/risks?applications=1 doesn't keep the order defined by "Category[order]" and Question[order]. Is it possible to sort the response so it keeps the original Categories and Questions order? :+1:
jonathanvila commented 3 years ago
  • In GET /assessments/risks?applications=1: could we change the QueryParam name applications by applicationId? we need to keep consistency with the other endpoints; all the other endpoints are using either assessmentId or applicationId as Query params. Good idea.
jonathanvila commented 3 years ago
  • We are using QueryParams for setting the list of applications so that info is used within the URL. We need to keep in mind that a URL's length is not unlimited. The info I've found is that a REST URL can be defined as up to 4096 characters which means only around 292 applications can be processed at a time: the queryParam is applications=1 (14 characters) => 4096/14=292.57 . I suppose this is a question for @PhilipCattanach : How many applications at a single time we are going to process? If the number is big, then it might be better to change the review endpoint to a POST request and sending the list of application within the body, but as I mentioned this really depends on how many apps we expect to process.
  • If the answer of "how many apps we are going to process at a time?" is "big numbers of apps" then it might be better to implement pagination in the backend for this endpoint; otherwise, let's keep it as it is and the UI can manage the pagination.

Yep, that's definitely a problem with all reports involving Applications. And this is one of them. I haven't changed the way the list is passed to the endpoint to make very CLEAR that this approach has a lot of problems. If the user can send hundreds/thousands of applications to the endpoint the problem is not if we pass them by Query String or Body, the problem is how the user can select manually hundreds/thousands of applications ( UI paginating every 25 elements ? how to decide which apps to include and which not : based on app name ? considering that one user entered the apps and another weeks later is asking reports , also everytime an user wants a report needs to send hundreds of apps ?... etc ) I think all of this has to be re-consider not using the individual application as an element to be requested but something that groups them and is easy to identify by an user.

carlosthe19916 commented 3 years ago

Thanks @jonathanvila . This PR looks good to me.