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 22, 2021

Motivation

Current quick start 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:

  • Changes the quickstart's database to be a Deployment of multi-arch postgres image
  • Changes the quickstart's application to be a Deployment of multi-arch image of Spring Petclinic including UI
  • Introduces the operator-backed database scenario as an alternative scenario for users with OLM
  • Extract pieces of the quickstart as re-usable parts and reuses them in both scenarios
  • Includes content of various source files directly in the docs to keep the docs in sync

Testing

Docs

Run make site to generate a preview and open out/site/userguide/getting-started/quick-start.html

Scenarios

Run included acceptance tests (tagged @getting-started).

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Dec 22, 2021

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@openshift-ci openshift-ci bot requested review from baijum and isutton December 22, 2021 13:52
@codecov
Copy link

codecov bot commented Dec 22, 2021

Codecov Report

Merging #1085 (82bdc83) into master (a796243) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1085   +/-   ##
=======================================
  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 a796243...82bdc83. Read the comment docs.

@pmacik pmacik force-pushed the refactor-quickstart branch 7 times, most recently from f13fca7 to 664da34 Compare January 11, 2022 14:10
@pmacik pmacik marked this pull request as ready for review January 11, 2022 14:10
@pmacik pmacik changed the title WIP: Refactor getting started guide to use the sample spring-petclinic Refactor getting started guide to use the sample spring-petclinic Jan 11, 2022
@pmacik pmacik force-pushed the refactor-quickstart branch from 664da34 to b7e26c7 Compare January 12, 2022 16:42
@pmacik
Copy link
Contributor Author

pmacik commented Jan 12, 2022

Rebased for fixed 4.10 acceptance tests (#1090)

@pmacik pmacik force-pushed the refactor-quickstart branch 3 times, most recently from f27bcbe to 80f0520 Compare January 12, 2022 17:02
@pedjak pedjak removed the request for review from isutton January 13, 2022 10:37
@pedjak
Copy link
Contributor

pedjak commented Jan 13, 2022

@Srivaralakshmi can you take a look as well?

@pedjak pedjak requested a review from sadlerap January 13, 2022 10:38
@pedjak
Copy link
Contributor

pedjak commented Jan 13, 2022

Also, please update the PR description and update Motivation part (current contains the placeholder text)

@Srivaralakshmi
Copy link
Contributor

Srivaralakshmi commented Jan 13, 2022

Hi, @pmacik: I have only reviewed 50% of the content. I am stopping now, as there are multiple files with the same content (for pgcluster and PostgreSQL). It is becoming very confusing for me to review as there is no preview. So stopping for now. Hope we can meet sometime today and discuss.

@pmacik pmacik force-pushed the refactor-quickstart branch 2 times, most recently from 0388d46 to d56e8b2 Compare January 13, 2022 13:44
@pmacik
Copy link
Contributor Author

pmacik commented Jan 27, 2022

Rebased on latest master

@pmacik
Copy link
Contributor Author

pmacik commented Jan 27, 2022

/retest

1 similar comment
@pmacik
Copy link
Contributor Author

pmacik commented Jan 27, 2022

/retest

Copy link
Contributor

@Srivaralakshmi Srivaralakshmi left a comment

Choose a reason for hiding this comment

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

Hi, @pmacik: Nice work. Thanks for incorporating the comments. I have left some minor suggestions. Otherwise, LGTM. Thanks!

@pmacik
Copy link
Contributor Author

pmacik commented Jan 27, 2022

/retest

@pmacik pmacik force-pushed the refactor-quickstart branch from 0efff16 to 777add4 Compare January 27, 2022 14:36
@pmacik
Copy link
Contributor Author

pmacik commented Jan 27, 2022

@Srivaralakshmi I've addressed all the review comments. Thank you for all your feedback!

Copy link
Contributor

@Srivaralakshmi Srivaralakshmi left a comment

Choose a reason for hiding this comment

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

Thanks, @pmacik for your patience and time, LGTM :)

@Srivaralakshmi
Copy link
Contributor

/lgtm

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jan 31, 2022

@Srivaralakshmi: changing LGTM is restricted to collaborators

In response to this:

/lgtm

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.

@pedjak
Copy link
Contributor

pedjak commented Jan 31, 2022

@pmacik IMHO this PR should contain also an acceptance tests verifying that documented quick start scenario works. This would enable catching regressions and mismatch between the doc and the user experience.

@pmacik
Copy link
Contributor Author

pmacik commented Feb 1, 2022

That's the plan, I wanted to finalize the quickstart before the acceptance testing - working on it.

@pmacik pmacik force-pushed the refactor-quickstart branch 2 times, most recently from 4ca581b to 73e3aac Compare February 1, 2022 18:12
@pmacik
Copy link
Contributor Author

pmacik commented Feb 1, 2022

Included acceptance tests to verify the quickstart scenarios

@pmacik
Copy link
Contributor Author

pmacik commented Feb 1, 2022

/retest

@pmacik pmacik force-pushed the refactor-quickstart branch from 73e3aac to 82bdc83 Compare February 1, 2022 18:49
@pmacik
Copy link
Contributor Author

pmacik commented Feb 1, 2022

/retest


[IMPORTANT]
====
The `Deployment` resource of the database has a couple of annotations set in the `.metadata.annotations` section:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
The `Deployment` resource of the database has a couple of annotations set in the `.metadata.annotations` section:
The `Deployment` resource of the database has few annotations set in the `.metadata.annotations` section:

Copy link
Contributor

@Srivaralakshmi Srivaralakshmi Feb 3, 2022

Choose a reason for hiding this comment

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

It is actually: The Deployment resource of the database has a few annotations set in the .metadata.annotations section:

It is a minor comment. Considering that this is the only comment that is pending and I am also working on a PR for the upstream content, if it helps, I can make this change along with my work. But this will take some time as I am working on the QSG content for OCP docs. I hope that is ok. @pmacik I may need your help if I run into errors during acceptance tests.

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

@baijum
Copy link
Contributor

baijum commented Feb 4, 2022

/approve

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Feb 4, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: baijum

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

@openshift-ci openshift-ci bot added the approved label Feb 4, 2022
@openshift-merge-robot openshift-merge-robot merged commit e0859ea into redhat-developer:master Feb 4, 2022
@pmacik pmacik linked an issue Feb 11, 2022 that may be closed by this pull request
@pmacik pmacik deleted the refactor-quickstart branch May 11, 2022 10:58
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved kind/documentation Improvements or additions to documentation lgtm
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Run Spring PetClinic Sample Application
5 participants