tackle-123-copy-assessment-between-2-apps

[jonathanvila]

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

Features covered

• New endpoint POST /assessments/copy/sourceApplicationId={appId}&targetApplicationId={appId} that will copy the assessment of the source application into the target application.

Unit , Integration and End2End Tests cases covered

• Integration Test
  • given_ApplicationAssessed_When_CopyAssessmentToAnotherNonAssessedApp_Then_NewAssessmentIsCreatedForTargetApplicationWithSameValues
  • given_ApplicationAssessed_When_CopyAssessmentToAnotherAssessedApp_Then_BadRequestIsAssessed
  • given_ApplicationAssessedButDeleted_When_CopyAssessmentToAnotherAssessedApp_Then_404NotFoundIsReturned
  • given_ApplicationNotAssessed_When_CopyAssessmentToAnotherNotAssessedApp_Then_BadRequestIsAssessed
• Unit Test
  • given_AlreadyAssessedApplication_When_CopyAssessment_ShouldCopyAllUserFieldValues
• End2End Test on minikube with native image
  • Given an application assessed, copy the assessment to another application

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 ++++++

NOTE

Script Test step number involved in this feature : 13,14

[carlosthe19916]

@jonathanvila @m-brophy my two cents:

• For starting a copy the target and source data is being inserted through QueryParams. https://github.com/konveyor/tackle-pathfinder/blob/170dc8edb2ad3a62fcfeaccd9e29eb729c524b7d/src/main/java/io/tackle/pathfinder/controllers/AssessmentsResource.java#L75 My opinion is that in POST/PUT operations we should be consistent with the rest of the endpoints and pass the data within the body and not through QueryParams. Technically nothing stops us from using QueryParams but I think we should have at least basic standards. The standard I propose is use queryParams if they are optional values, use the body or pathParams if the data is mandatory. Wdyt?
• For copying an assessment the current PR is using sourceApplicationId and targetApplicationId but I think what we should send to the backend is sourceAssessmentId and targetApplicationIds using /POST /assessments/{sourceAssessmentId}/copy and within the body of the request we should send the list of applicationIds we want to copy the assessment to. e.g:

  POST /assessments/1/copy
  {
  applicationIdTarget: [1,2,3,4,5,6]
  }

  Of course for the current PR we should only copy assessments one by one but for the future, I think we should copy multiple assessments.

[jonathanvila]

My opinion is that in POST/PUT operations we should be consistent with the rest of the endpoints and pass the data within the body and not through QueryParams. Technically nothing stops us from using QueryParams but I think we should have at least basic standards. The standard I propose is use queryParams if they are optional values, use the body or pathParams if the data is mandatory. Wdyt?

I would say Queryparams can be used when are not part of an entity to be stored. Optional/Mandatory should be the criteria to use query params in my opinion. Few criterias :

• if you talk about 2,3 fields then Querystring could be a good place
• if you are sending DATA (not params) it should go in the body
• if you are sending parameters that configure an opperation, querystring could be a good place

• Here there are few links that can support current approach : https://stackoverflow.com/questions/25385559/rest-api-best-practices-args-in-query-string-vs-in-request-body https://www.moesif.com/blog/technical/api-design/REST-API-Design-Best-Practices-for-Parameters-and-Query-String-Usage/

Obviously everything is opinionable in API design.

If from the UI point of view is easier using body content, we can modify it.

[jonathanvila]

• For copying an assessment the current PR is using sourceApplicationId and targetApplicationId but I think what we should send to the backend is sourceAssessmentId and targetApplicationIds using /POST /assessments/{sourceAssessmentId}/copy and within the body of the request we should send the list of applicationIds we want to copy the assessment to. e.g:

POST /assessments/1/copy
{
 applicationIdTarget: [1,2,3,4,5,6]
}

Of course for the current PR we should only copy assessments one by one but for the future, I think we should copy multiple assessments.

The current scope for this PR is copying one by one. There's another Ticket to cover the bulk copy, that will need a little discussion to cover ( https://github.com/konveyor/tackle-pathfinder/issues/38 ) :

• sync or async , as it can take long to copy to several apps
• response as a list with status for each target app, as it could happen a subgroup of target apps fails , due to they already have an assessment, process exceptions, etc.

Appartfrom that the url you are proposing is copying an assessment NOT an application. From the UI perspective it is selecting an application not an assessment. The right way to do it would be : /application/1234/copy

But agains, this is totally opinable as the difference is very subtile.

[m-brophy]

My comments have all been addressed satisfactorily so when you are happy @carlosthe19916 can you merge this PR?