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

Updating the Service binding content #23726

Merged
merged 1 commit into from
Feb 21, 2022
Merged

Updating the Service binding content #23726

merged 1 commit into from
Feb 21, 2022

Conversation

MichalMaler
Copy link
Contributor

A draft for a possible update of this chapter has been created under cooperation with Ioannis Kanellos.

Signed-off-by: Michal Maléř [email protected]

@geoand geoand requested a review from iocanel February 15, 2022 14:30
iocanel
iocanel previously requested changes Feb 15, 2022
View reviewed changes
Copy link
Contributor

@iocanel iocanel left a comment

Choose a reason for hiding this comment

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

Overall looks, good.
There are two chunks where I prefer the original text over the modified one.

geoand
geoand reviewed Feb 16, 2022
View reviewed changes
docs/src/main/asciidoc/deploying-to-kubernetes.adoc Outdated
<artifactId>quarkus-hibernate-orm</artifactId>
</dependency>
----
Users often use this concept of workload projection during the development time to connect their application to a dev-database, or other locally runned services, without changing the actual application code or configuration.
Copy link
Contributor

Choose a reason for hiding this comment

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

Where does this come from? I personally don't know anyone that does this...

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 have got this from my latest meeting with Ioannis. @iocanel
Maybe I just misunderstood something but thats what we got in the google doc

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, I have an upgrade for this:

During application development, users can use workload projection to connect their application to a dev-database, or other locally-run services, without changing the actual application code or configuration.

Copy link
Contributor

Choose a reason for hiding this comment

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

This comes from me. I am not sure how extensively this pattern is used, but my understanding is that one of the benefits of workload projection compared to the traditional evn variable style of binding is the fact that it can be simpler to use in development, testing etc.

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 being security

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, I added it since I find it valuable for the section.

geoand
geoand reviewed Feb 17, 2022
View reviewed changes
Copy link
Contributor

@geoand geoand left a comment

Choose a reason for hiding this comment

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

Looks better!

@iocanel has the final say on this

iocanel
iocanel reviewed Feb 17, 2022
View reviewed changes
docs/src/main/asciidoc/deploying-to-kubernetes.adoc Outdated
<artifactId>quarkus-hibernate-orm</artifactId>
</dependency>
----
Users often use this concept of workload projection during the development time to connect their application to a dev-database, or other locally runned services, without changing the actual application code or configuration.
Copy link
Contributor

Choose a reason for hiding this comment

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

This comes from me. I am not sure how extensively this pattern is used, but my understanding is that one of the benefits of workload projection compared to the traditional evn variable style of binding is the fact that it can be simpler to use in development, testing etc.

@MichalMaler
Copy link
Contributor Author

I have one last suggestion regarding of our headings in here:
== Introduction to the Service Binding Operator instead of Using the Service Binding Operator

Just because action oriented headings should be only used for Procedures.
Let me know :)

@MichalMaler
Copy link
Contributor Author

I applied some minor language changes. My part is done for now. @geoand @iocanel

What we can decide further on this subject is:

  1. If @iocanel likes the original text more in the intro part, maybe he could reuse the original content, slightly enhance with what I have added, and add this as a single commit? I intended to update and enhance but also, to make Upstream satisfied and happy about it :) Master touch needs to be done, I guess. Glad to cooperate in any way.

  2. In the googleDoc we used for drafting, we have some questions from my college writer. It would be great to address them and enhance the content if needed.

Thank you so much for great cooperation and kind feedback provided by @iocanel

geoand
geoand reviewed Feb 18, 2022
View reviewed changes
Copy link
Contributor

@geoand geoand left a comment

Choose a reason for hiding this comment

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

Thanks for this.

We will need the commits to be squashed

@MichalMaler
Copy link
Contributor Author

Thanks for this.

We will need the commits to be squashed

Sure, no problem. Will do when we will get the green light from @iocanel so that no more commits will be needed.

@geoand
Copy link
Contributor

geoand commented Feb 18, 2022

Makes sense!

@iocanel
Copy link
Contributor

iocanel commented Feb 21, 2022

Added some minor changes to the doc:

  • Added link and overview of the Service Binding Operator.
  • Use the crunchy data postgres operator all across the board.
  • Simplify intro part.

@iocanel
Copy link
Contributor

iocanel commented Feb 21, 2022

@geoand: It looks good to me. Can you please review my changes?

geoand
geoand approved these changes Feb 21, 2022
View reviewed changes
Copy link
Contributor

@geoand geoand left a comment

Choose a reason for hiding this comment

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

LGTM

@iocanel
Copy link
Contributor

iocanel commented Feb 21, 2022

@MichalMaler can you please squash all changes including mine, so that we can merge?

@MichalMaler
Copy link
Contributor Author

@MichalMaler can you please squash all changes including mine, so that we can merge?

Will do! With a pleasure :) But sirs, I need to say that this chapter will be so good now, that it wont be fair in comparison with the rest of the document :D Great work all of you.

@MichalMaler MichalMaler force-pushed the Updating-the-Service-binding-content branch 2 times, most recently from 40ea137 to 5e6a891 Compare February 21, 2022 10:38
@MichalMaler MichalMaler requested a review from iocanel February 21, 2022 10:38
@MichalMaler MichalMaler force-pushed the Updating-the-Service-binding-content branch from 5e6a891 to 2b1c18f Compare February 21, 2022 10:42
@geoand geoand dismissed iocanel’s stale review February 21, 2022 10:43

comments addresed

@geoand geoand added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Feb 21, 2022
@MichalMaler MichalMaler force-pushed the Updating-the-Service-binding-content branch from 2b1c18f to db1ad62 Compare February 21, 2022 10:45
@MichalMaler
Copy link
Contributor Author

Reviewed, squashed, rebased.

@MichalMaler MichalMaler force-pushed the Updating-the-Service-binding-content branch from db1ad62 to 5f374b6 Compare February 21, 2022 11:00
@MichalMaler
Doc: Updating the Service Binding content
33c74b9 
Signed-off-by: Michal Maléř <[email protected]>

Applying suggestions from the SME review

Signed-off-by: Michal Maléř <[email protected]>

Vale fixes

Signed-off-by: Michal Maléř <[email protected]>

Style tuning

Signed-off-by: Michal Maléř <[email protected]>

Applying suggestions from the SME review

Signed-off-by: Michal Maléř <[email protected]>

Language fixes

Signed-off-by: Michal Maléř <[email protected]>

Language fixes

Signed-off-by: Michal Maléř <[email protected]>

doc: minor changes in service binding doc
@MichalMaler MichalMaler force-pushed the Updating-the-Service-binding-content branch from 5f374b6 to 33c74b9 Compare February 21, 2022 11:00
@geoand geoand merged commit 84e3b01 into quarkusio:main Feb 21, 2022
@quarkus-bot quarkus-bot bot added this to the 2.8 - main milestone Feb 21, 2022
@quarkus-bot quarkus-bot bot removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label Feb 21, 2022
@gsmet gsmet modified the milestones: 2.8 - main, 2.7.2.Final Feb 21, 2022
@MichalMaler MichalMaler deleted the Updating-the-Service-binding-content branch February 22, 2022 11:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@geoand geoand geoand approved these changes

@iocanel iocanel Awaiting requested review from iocanel

Assignees
No one assigned
Labels
area/documentation
Projects
None yet
Milestone
2.7.2.Final
Development

Successfully merging this pull request may close these issues.
4 participants
@MichalMaler @geoand @iocanel @gsmet