Skip to content

Conversation

@abhatt-rh
Copy link
Collaborator

This PR converts the Validated Patterns Operator file to asciidoc and implements style and consistency changes

Issues:

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 17, 2023

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: abhatt-rh

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

Details 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

@abhatt-rh abhatt-rh changed the title WIP: Converts the Validated Patterns Operator file to asciidoc and review for changes Converts the Validated Patterns Operator file to asciidoc and review for changes Aug 29, 2023
@abhatt-rh
Copy link
Collaborator Author

abhatt-rh commented Aug 29, 2023

Hi @danmacpherson and @mbaldessari,
Kindly review the converted and revised content for the Validated Patterns Operator. For the purpose of reviewing/comparing with original this PR and since I have replaced all the screen caps with content, I have not yet replaced the .md file with the converted .adoc file. After the review is complete and I have implemented your comments, we can remove the .md file.
Thanks!

@danmacpherson
Copy link
Collaborator

No major issues from my side. This is just the conversion and not the style and consistency review, right? If so, then:

/lgtm

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 30, 2023

New changes are detected. LGTM label has been removed.

@abhatt-rh abhatt-rh force-pushed the td-1431 branch 2 times, most recently from 3b2ca34 to 9ff47d2 Compare August 30, 2023 11:46
@abhatt-rh
Copy link
Collaborator Author

No major issues from my side. This is just the conversion and not the style and consistency review, right? If so, then:

/lgtm

Hi @danmacpherson - This PR addresses both, the conversion and the style and consistency.

@danmacpherson
Copy link
Collaborator

Ah gotcha, I'll take a closer look after our vF2F.

Copy link
Collaborator

@danmacpherson danmacpherson left a comment

Choose a reason for hiding this comment

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

Some minor feedback to implement. Otherwise, it's all good.

+
To know the cluster group name for the patterns that you want to deploy, check the relevant pattern-specific requirements.
. Expand the *Git Config* section to reveal the options and enter the required information.
.. Change the *Target Repo* URL to your forked repository URL. For example, `+https://github.com/validatedpatterns/<pattern_name>+` to `+https://github.com/<your-git-username>/<pattern-name>+`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does the the default actually say https://github.com/validatedpatterns/<pattern_name>? If so, this is a minor concern but users might also interpret https://github.com/<your-git-username>/<pattern-name> to be literal and not replaceable since it's following the same pattern as the default. Maybe we need to revise this line to also say to replace the <your-git-username> and <pattern-name> with their actual user name and the pattern name? Or am I overthinking this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, it does not literally say https://github.com/validatedpatterns/<pattern_name>. The only variable is the pattern-name and hence that syntax <>.

Not sure if we need to spell that out but take a look at the actual screen capture and if you still feel we need to clarify it further, we can update the description to say so.
image

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah I understand now. Might need an additional verb to make that last line a complete sentence.

Suggested change
.. Change the *Target Repo* URL to your forked repository URL. For example, `+https://github.com/validatedpatterns/<pattern_name>+` to `+https://github.com/<your-git-username>/<pattern-name>+`
.. Change the *Target Repo* URL to your forked repository URL. For example, change `+https://github.com/validatedpatterns/<pattern_name>+` to `+https://github.com/<your-git-username>/<pattern-name>+`

Copy link
Contributor

@mbaldessari mbaldessari left a comment

Choose a reason for hiding this comment

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

lgtm overall

menu:
learn:
parent: Infrastructure
title: Using the Validated Patterns Operator (asciidoc)
Copy link
Contributor

Choose a reason for hiding this comment

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

Drop the '(asciidoc)' before we merge it? Or do you want to kill the old md file first and then drop it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hi @mbaldessari. Yes, I'll drop the asciidoc before I merge it and delete the old file (.md) at the same time.

Copy link
Collaborator Author

@abhatt-rh abhatt-rh left a comment

Choose a reason for hiding this comment

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

Hi @danmacpherson and @mbaldessari for your review.
I have addressed your comments. PTAL. The build is failing for known issues (mostly links to the older repo, which eventually redirect to the new site). Yet, i'll get them fixed in a follow-up PR

+
To know the cluster group name for the patterns that you want to deploy, check the relevant pattern-specific requirements.
. Expand the *Git Config* section to reveal the options and enter the required information.
.. Change the *Target Repo* URL to your forked repository URL. For example, `+https://github.com/validatedpatterns/<pattern_name>+` to `+https://github.com/<your-git-username>/<pattern-name>+`
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, it does not literally say https://github.com/validatedpatterns/<pattern_name>. The only variable is the pattern-name and hence that syntax <>.

Not sure if we need to spell that out but take a look at the actual screen capture and if you still feel we need to clarify it further, we can update the description to say so.
image

@abhatt-rh
Copy link
Collaborator Author

Ignoring the failed HTMLTest check and merging this PR. Will address the failure in a follow-up PR

@abhatt-rh abhatt-rh merged commit bddac18 into validatedpatterns:main Sep 4, 2023
@abhatt-rh abhatt-rh deleted the td-1431 branch September 15, 2023 17:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants