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

A few minor tweaks to recent updates #434

Merged
merged 1 commit into from
Feb 2, 2024
Merged

A few minor tweaks to recent updates #434

merged 1 commit into from
Feb 2, 2024

Conversation

bergerhoffer
Copy link
Collaborator

Just a few tweaks I noticed. The incorrect forms should be single words/phrases, not sentences

@bergerhoffer bergerhoffer added the General update General updates to the guide or repo label Jan 9, 2024
mportman12
mportman12 previously approved these changes Jan 10, 2024
View reviewed changes
Copy link
Collaborator

@mportman12 mportman12 left a comment

Choose a reason for hiding this comment

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

LGTM

@buildbricks
Copy link

LGTM

sbmetz
sbmetz previously approved these changes Jan 16, 2024
View reviewed changes
Copy link
Contributor

@sbmetz sbmetz left a comment

Choose a reason for hiding this comment

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

LGTM

@buildbricks
Copy link

@bergerhoffer This is ready

rolfedh
rolfedh reviewed Jan 16, 2024
View reviewed changes
supplementary_style_guide/glossary_terms_conventions/general_conventions/i.adoc Outdated
@@ -406,11 +406,12 @@ There is no functional difference between the first server that was installed an
==== image:images/yes.png[yes] ingress (noun, adjective)
*Description*: In Red Hat OpenShift, _Ingress_ is a Kubernetes API object that developers can use to expose services within the cluster through an HTTP(S) load balancer and a proxy layer by using a public DNS entry. The `Ingress` resource defines the cluster-wide configuration for ingress traffic, and provides the ability to specify TLS options, a certificate, or a public CNAME that the OpenShift `IngressController` object can accept for HTTP(S) traffic. Additionally, _ingress_ can also be used to describe the incoming direction of network traffic. In Red Hat OpenShift, for example, this traffic is described as entering (ingress) or leaving (egress) an OpenShift cluster.

Do not use _Ingress_ without markup, when referencing the `Ingress` resource or the `IngressController` object. Do not use _Ingress_ with markup when used to describe the direction of traffic.
Copy link
Contributor

@rolfedh rolfedh Jan 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Removing the comma:

Suggested change
Do not use _Ingress_ without markup, when referencing the `Ingress` resource or the `IngressController` object. Do not use _Ingress_ with markup when used to describe the direction of traffic.
Do not use _Ingress_ without markup when referencing the `Ingress` resource or the `IngressController` object. Do not use _Ingress_ with markup when used to describe the direction of traffic.

Maybe:

Suggested change
Do not use _Ingress_ without markup, when referencing the `Ingress` resource or the `IngressController` object. Do not use _Ingress_ with markup when used to describe the direction of traffic.
Always use _Ingress_ with markup when referencing the `Ingress` resource or `IngressController` object in OpenShift. Omit markup when discussing _ingress_ as a traffic direction.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@skrthomas What do you think of the adjustment the @rolfedh has here (the second one)?

I'd go with "Red Hat OpenShift" instead of just "OpenShift", but otherwise it LGTM. (Or maybe we need to also call out that it should be lowercase when it's the direction of traffic. wdyt?

Copy link
Contributor

@skrthomas skrthomas Jan 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@bergerhoffer Oops, I am so sorry I somehow missed this in my GitHub inbox probably due to peer review squad influx 🙃 I think @rolfedh makes a great suggestion that clarifies this statement a lot. I'm personally fine with just OpenShift. I would agree that we can callout ingress needing to be lowercase when its directional traffic.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks @skrthomas, updated!

rolfedh
rolfedh previously approved these changes Jan 16, 2024
View reviewed changes
Copy link
Contributor

@rolfedh rolfedh left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM. I've offered a minor typo fix and an alternative version.

buildbricks
buildbricks previously approved these changes Jan 17, 2024
View reviewed changes
Copy link

@buildbricks buildbricks left a comment

Choose a reason for hiding this comment

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

lgtm

@buildbricks
Copy link

@bergerhoffer PTAL

@Srivaralakshmi
Copy link
Contributor

@skrthomas Can you PTAL at this comment and respond?

This PR is very close to getting ready and merged. Please help. TY!

@skrthomas
Copy link
Contributor

skrthomas commented Jan 31, 2024 •
edited
Loading

Added a comment! It looks good to me and thanks @rolfedh @bergerhoffer for the tweaks :)

@bergerhoffer
Copy link
Collaborator Author

@rolfedh @joaedwar @mportman12 @sbmetz Updated per Rolfe's last suggestion, ready for re-review!

@Srivaralakshmi
Copy link
Contributor

Thanks @bergerhoffer for the revision!

@rolfedh @joaedwar @mportman12 @sbmetz Need two more approvals to merge this PR. Please help!

rolfedh
rolfedh approved these changes Feb 2, 2024
View reviewed changes
Copy link
Contributor

@rolfedh rolfedh left a comment

Choose a reason for hiding this comment

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

LGTM

mportman12
mportman12 approved these changes Feb 2, 2024
View reviewed changes
Copy link
Collaborator

@mportman12 mportman12 left a comment

Choose a reason for hiding this comment

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

LGTM

@Srivaralakshmi
Copy link
Contributor

LGTM

@Srivaralakshmi
Copy link
Contributor

3 approvals, merging

@Srivaralakshmi Srivaralakshmi merged commit 8e2465b into redhat-documentation:main Feb 2, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@skrthomas skrthomas skrthomas left review comments

@mportman12 mportman12 mportman12 approved these changes

@rolfedh rolfedh rolfedh approved these changes

@buildbricks buildbricks buildbricks left review comments

@sbmetz sbmetz sbmetz left review comments

Assignees
No one assigned
Labels
General update General updates to the guide or repo
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@bergerhoffer @buildbricks @Srivaralakshmi @skrthomas @rolfedh @sbmetz @mportman12