Skip to content
This repository was archived by the owner on Jun 26, 2024. It is now read-only.

Conversation

pmacik
Copy link
Contributor

@pmacik pmacik commented Dec 9, 2021

Motivation

Current quickstart uses only REST API server part of the Spring Petclinic as an application and Crunchy Postgresql Operator backed DB. It has couple of UX issues.

The current application:

  • does not have UI so user must use kubectl port-forward and curl command to send requests to the application's API server and analyze responses for accessing and verifying that the application works as expected
  • user has to manually run a script to initialize database with application schema and data (as one of the quickstart's steps)

The current database:

  • requires installation of Crunchy Postgresql Operator (via OLM) that wouldn't be possible for user with kubernetes cluster without OLM
  • uses an operator that is not available on other then x86_64 architectures, which excludes all users on alternative architectures

Changes

This PR:

Testing

The following steps involve running some convenient Makefile targets that serve here only to ease testing and will be replaced by the actual kubectl or oc commands and explained in the quickstart, eventually.

  1. Set your environment

    • with minikube (default)
      export KUBERNETES_RUNTIME=minikube
    • with OpenShift
      export KUBERNETES_RUNTIME=openshift
  2. Go to samples/app/spring-petclinic directory
    cd samples/app/spring-petclinic

  3. Create user namespace (my-postgresql by default)
    make namespace

  4. Deploy database Deployment
    make deploy-database

  5. Expose DB binding data by setting annotations on database Deployment
    make annotate-database

  6. Deploy application Deployment
    make deploy-app
    URL for the application UI is printed out so open it to access the application, but application's pod is stuck in Init state due unavailable DB.

  7. Bind application to db
    make bind

  8. After the application pod is restarted, the application's pod is Running UI should show the web interface.

@openshift-ci openshift-ci bot requested review from isutton and pedjak December 9, 2021 11:48
@pmacik pmacik force-pushed the sample-app-petclinic branch from 7172913 to 2052a82 Compare December 9, 2021 12:27
@pedjak pedjak requested review from sadlerap and removed request for isutton December 9, 2021 12:29
@pmacik
Copy link
Contributor Author

pmacik commented Dec 9, 2021

@yselkowitz could you please take a look in terms of multi-arch?
Especially the two application images to be built from included Dockerfiles

Thanks

@codecov
Copy link

codecov bot commented Dec 9, 2021

Codecov Report

Merging #1080 (33e9946) into master (36f271e) will not change coverage.
The diff coverage is n/a.

❗ Current head 33e9946 differs from pull request most recent head cf55b32. Consider uploading reports for the commit cf55b32 to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1080   +/-   ##
=======================================
  Coverage   58.92%   58.92%           
=======================================
  Files          30       30           
  Lines        1670     1670           
=======================================
  Hits          984      984           
  Misses        560      560           
  Partials      126      126           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 36f271e...cf55b32. Read the comment docs.

@pmacik pmacik force-pushed the sample-app-petclinic branch from 2052a82 to 853f855 Compare December 9, 2021 15:15
@pmacik
Copy link
Contributor Author

pmacik commented Dec 9, 2021

/retest

Copy link
Contributor

@pedjak pedjak left a comment

Choose a reason for hiding this comment

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

additionally, please avoid folder names starting with a dot, e.g. .minikube, .openshift.. they are hidden by default from shell and this can create some confusions.

Comment on lines 5 to 6
sed -e 's,REPLACE_PETCLINIC_APP_IMAGE,'${PETCLINIC_APP_IMAGE}',g' app-deployment.yaml | \
sed -e 's,REPLACE_PETCLINIC_APP_DBINIT_IMAGE,'${PETCLINIC_APP_DBINIT_IMAGE}',g' | \
Copy link
Contributor

Choose a reason for hiding this comment

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

the rest of the project uses kustomize, so if really needed we could use that instead of sed

@pmacik pmacik changed the title Introduce sample application (spring-petclinic + postgresql) to be used in quickstart [WIP] Introduce sample application (spring-petclinic + postgresql) to be used in quickstart Dec 17, 2021
@pmacik pmacik force-pushed the sample-app-petclinic branch 3 times, most recently from 33178c0 to 33e9946 Compare December 17, 2021 14:19
@pmacik
Copy link
Contributor Author

pmacik commented Dec 17, 2021

/retest

@pmacik
Copy link
Contributor Author

pmacik commented Dec 19, 2021

Sent a pull-request spring-projects/spring-petclinic#887 to enable spring cloud bindings in the petclinic application to avoid the need of patching the app source while building the app container.

@pmacik pmacik force-pushed the sample-app-petclinic branch 2 times, most recently from 60490c8 to 7b53d08 Compare December 21, 2021 11:08
@pmacik pmacik force-pushed the sample-app-petclinic branch 2 times, most recently from df6a697 to 213e76d Compare December 21, 2021 11:30
@pmacik pmacik changed the title [WIP] Introduce sample application (spring-petclinic + postgresql) to be used in quickstart Introduce sample application (spring-petclinic + postgresql) to be used in quickstart Dec 21, 2021
@pmacik pmacik force-pushed the sample-app-petclinic branch 2 times, most recently from ceaf104 to cf7ef67 Compare December 21, 2021 19:26
@pmacik
Copy link
Contributor Author

pmacik commented Dec 22, 2021

/retest

Copy link
Contributor

@pedjak pedjak left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Dec 22, 2021
Copy link
Contributor

@baijum baijum left a comment

Choose a reason for hiding this comment

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

/lgtm

@pedjak
Copy link
Contributor

pedjak commented Jan 10, 2022

/approve

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jan 10, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: pedjak

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@pmacik pmacik force-pushed the sample-app-petclinic branch from cf7ef67 to 1a681b6 Compare January 10, 2022 14:53
@openshift-ci openshift-ci bot removed the lgtm label Jan 10, 2022
@pedjak
Copy link
Contributor

pedjak commented Jan 10, 2022

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Jan 10, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jan 10, 2022

@pmacik: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/4.10-acceptance 1a681b6 link true /test 4.10-acceptance

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@pmacik
Copy link
Contributor Author

pmacik commented Jan 11, 2022

The Bind test application to Postgres instance provisioned by Cloud Native Postgres operator test fails for OpenShift 4.10 because the Cloud Native Postgres operator is not available in 4.10 catalog.

@pmacik pmacik merged commit df645fc into redhat-developer:master Jan 11, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants