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

SiteConfig: fix rendering issues #7209

Merged
merged 1 commit into from
Nov 4, 2024
Merged

SiteConfig: fix rendering issues #7209

merged 1 commit into from
Nov 4, 2024

Conversation

amolnar-rh
Copy link
Collaborator

@amolnar-rh amolnar-rh commented Nov 1, 2024 •
edited
Loading

dockerymick
dockerymick requested changes Nov 1, 2024
View reviewed changes
Copy link
Contributor

@dockerymick dockerymick left a comment

Choose a reason for hiding this comment

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

Left one comment and verified that the links work. Were the steps not rendering correctly in siteconfig_scale_out_worker_nodes.adoc?

mce_acm_integration/siteconfig/siteconfig_scale_out_worker_nodes.adoc Outdated
@@ -78,8 +78,7 @@ worker-node2.example.com provisioning true

. Verify that the `Node`, `Agent`, and `BareMetalHost` resources are created.

.. Verify that the `Node` resources are removed by running the following command on the hub cluster:

.. Verify that the `Node` resources are removed by running the following command on the hub cluster:
Copy link
Contributor

Choose a reason for hiding this comment

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

I recommend that you revert the change here. There should be a new line between the step and plus sign

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It doesn't look right and I'm checking the preview in my local editor. The only way to resolve the issue of the codeblocks is by removing the extra space.
Screenshot from 2024-11-01 17-05-50

Screenshot from 2024-11-01 13-16-05

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Anyway, I had to change this section but there are other steps where it's not rendering right:
Screenshot from 2024-11-01 17-08-31

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, let's leave it for now and. we can revisit in a refresh

@amolnar-rh amolnar-rh force-pushed the siteconfig-last-fixes branch 4 times, most recently from cd89514 to b11db22 Compare November 1, 2024 17:19
dockerymick
dockerymick requested changes Nov 1, 2024
View reviewed changes
Copy link
Contributor

@dockerymick dockerymick 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 your hard work on this! I left a few suggestions

apis/clusterinstance.json.adoc Outdated Show resolved Hide resolved
mce_acm_integration/siteconfig/ibio_intro.adoc Outdated
@@ -3,5 +3,5 @@

Install the {ibio} so that you can complete and manage image-based cluster installations by using the same APIs as existing installation methods.

For more information about, and to learn how to enable the {ibio}, see link:https://docs.redhat.com/en/documentation/openshift_container_platform/4.17/html/edge_computing/image-based-installation-for-single-node-openshift[Image-based installations for {sno}].
For more information about, and to learn how to enable the {ibio}, see https://docs.redhat.com/en/documentation/openshift_container_platform/4.17/html/edge_computing/image_base_install/ibi_deploying_sno_clusters/ibi-edge-image-based-install[Deploying managed {sno} using the IBI Operator].
Copy link
Contributor

Choose a reason for hiding this comment

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

this link is also broken we also need link: for the syntax. I think that is not the reason why the link is broken, just making the suggestion so that the link references are consistent

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For visibility, I'm commenting here as well that the link will be broken until the content is released in the OCP docs that should happen at the same time as the ACM 2.12 GA. This link might need a fix after release

mce_acm_integration/siteconfig/siteconfig_enable.adoc Outdated Show resolved Hide resolved
mce_acm_integration/siteconfig/siteconfig_install_clusters.adoc Outdated Show resolved Hide resolved
mce_acm_integration/siteconfig/siteconfig_install_clusters.adoc Outdated Show resolved Hide resolved
mce_acm_integration/siteconfig/siteconfig_scale_in_worker_nodes.adoc Outdated Show resolved Hide resolved
mce_acm_integration/siteconfig/siteconfig_scale_in_worker_nodes.adoc Outdated Show resolved Hide resolved
mce_acm_integration/siteconfig/siteconfig_scale_in_worker_nodes.adoc Outdated Show resolved Hide resolved
mce_acm_integration/siteconfig/siteconfig_scale_out_worker_nodes.adoc Outdated Show resolved Hide resolved
mce_acm_integration/siteconfig/siteconfig_scale_out_worker_nodes.adoc Outdated Show resolved Hide resolved
dockerymick
dockerymick requested changes Nov 1, 2024
View reviewed changes
Copy link
Contributor

@dockerymick dockerymick left a comment

Choose a reason for hiding this comment

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

Couple more comments

mce_acm_integration/siteconfig/siteconfig_install_clusters.adoc Outdated Show resolved Hide resolved
mce_acm_integration/siteconfig/siteconfig_install_clusters.adoc Show resolved Hide resolved
mce_acm_integration/siteconfig/siteconfig_installation_templates.adoc Outdated Show resolved Hide resolved
mce_acm_integration/siteconfig/siteconfig_scale_out_worker_nodes.adoc Outdated Show resolved Hide resolved
dockerymick
dockerymick approved these changes Nov 1, 2024
View reviewed changes
Copy link
Contributor

@dockerymick dockerymick left a comment

Choose a reason for hiding this comment

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

Besides the comment about the command, this looks good to me!

@openshift-ci openshift-ci bot added the lgtm label Nov 1, 2024
@openshift-ci OpenShift CI
Copy link

openshift-ci bot commented Nov 1, 2024

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: amolnar-rh, dockerymick

The full list of commands accepted by this bot can be found 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

sakhoury
sakhoury suggested changes Nov 4, 2024
View reviewed changes
Copy link

@sakhoury sakhoury 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 the great work @amolnar-rh 🎉 🚀

apis/clusterinstance.json.adoc Show resolved Hide resolved
mce_acm_integration/siteconfig/siteconfig_scale_in_worker_nodes.adoc Outdated

+
[source,terminal]
----
oc get bmh -n <managed_cluster_namespace> --watch
oc get bmh -n <clusterinstance_namespace> worker-node2.example.com -ojsonpath='{.metadata.annotations}' | jq
Copy link

Choose a reason for hiding this comment

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

The additional space is not required - good catch @dockerymick!

mce_acm_integration/siteconfig/siteconfig_scale_in_worker_nodes.adoc Outdated Show resolved Hide resolved
mce_acm_integration/siteconfig/siteconfig_scale_out_worker_nodes.adoc Outdated Show resolved Hide resolved
mce_acm_integration/siteconfig/siteconfig_scale_out_worker_nodes.adoc Outdated Show resolved Hide resolved
mce_acm_integration/siteconfig/siteconfig_scale_out_worker_nodes.adoc Outdated Show resolved Hide resolved
@openshift-ci openshift-ci bot removed the lgtm label Nov 4, 2024
@amolnar-rh amolnar-rh merged commit edeae0a into 2.12_stage Nov 4, 2024
1 check failed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sakhoury sakhoury sakhoury requested changes

@dockerymick dockerymick dockerymick approved these changes

Assignees

@sakhoury sakhoury

@dockerymick dockerymick

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@amolnar-rh @sakhoury @dockerymick