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.

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

Format the issuers table #1383

Merged
merged 2 commits into from
Jan 11, 2024
Merged

Conversation

wallrj
Copy link
Member

@wallrj wallrj commented Jan 11, 2024

While reviewing #1382 I wanted to help the author move their new issuer to the right place in the table,
but the markdown table is currently unaligned and difficult to read in the GitHub preview,
so here I'm trying to fix that problem first.

image

vs

image

@jetstack-bot jetstack-bot added dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 11, 2024
@wallrj wallrj force-pushed the format-issuers-table branch from 9adc633 to 6685981 Compare January 11, 2024 09:54
Copy link

netlify bot commented Jan 11, 2024

Deploy Preview for cert-manager-website ready!

Name Link
🔨 Latest commit 9adc633
🔍 Latest deploy log https://app.netlify.com/sites/cert-manager-website/deploys/659fba670ff2d100070ad507
😎 Deploy Preview https://deploy-preview-1383--cert-manager-website.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

netlify bot commented Jan 11, 2024

Deploy Preview for cert-manager-website ready!

Name Link
🔨 Latest commit 3c913f6
🔍 Latest deploy log https://app.netlify.com/sites/cert-manager-website/deploys/659fc4b80922960008ed3add
😎 Deploy Preview https://deploy-preview-1383--cert-manager-website.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@wallrj wallrj force-pushed the format-issuers-table branch 2 times, most recently from 748f3df to 35567e8 Compare January 11, 2024 10:30
Signed-off-by: Richard Wall <richard.wall@venafi.com>
@wallrj wallrj force-pushed the format-issuers-table branch from 35567e8 to 70761de Compare January 11, 2024 10:32
Signed-off-by: Richard Wall <richard.wall@venafi.com>
@@ -35,7 +37,7 @@ The following list contains all known cert-manager issuer integrations.
[//]: # (Configuration docs)

[config:venafi-enhanced-issuer]: https://docs.venafi.cloud/vaas/k8s-components/t-vei-install/
[config:acme-issuer]: ./acme.md
[config:acme-issuer]: ./acme/README.md
Copy link
Member Author

Choose a reason for hiding this comment

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

This broken link was picked up by the link checker.
Not sure why it hadn't highlighted it in earlier PRs.

[ca:cfssl-issuer]: https://github.com/cloudflare/cfssl
[ca:freeipa-issuer]: https://www.freeipa.org
[ca:kms-issuer]: https://aws.amazon.com/kms/
[ca:origin-ca-issuer]: https://developers.cloudflare.com/ssl/origin-configuration/origin-ca
Copy link
Member Author

Choose a reason for hiding this comment

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

I moved all the links to the CA documentation out of the table,
to make the table easier to read and for consistency with the other links in the table which are all aliased.

@wallrj wallrj requested a review from maelvls January 11, 2024 10:43
| 🥉 | freeipa-issuer | [📄][config:freeipa-issuer] | [FreeIPA][ca:freeipa-issuer] | - | [❌][release:freeipa-issuer] | ✔️ |
| 🥉 | kms-issuer | [📄][config:kms-issuer] | [AWS KMS][ca:kms-issuer] | - | [❌][release:kms-issuer] | ✔️ |
| 🥉 | origin-ca-issuer | [📄][config:origin-ca-issuer] | [Cloudflare Origin CA][ca:origin-ca-issuer] | - | [❌][release:origin-ca-issuer] | ✔️ |

Copy link
Member Author

Choose a reason for hiding this comment

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

This and the earlier blank line between the markdown table and the surrounding html tags are what fix the table rendering in GitHub file preview.

Copy link
Member

@maelvls maelvls Jan 11, 2024

Choose a reason for hiding this comment

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

Thanks for the context. I have myself hit this issue of broken table due to an html tag too close to the bottom of the table multiple times.

@maelvls
Copy link
Member

maelvls commented Jan 11, 2024

/lgtm
/approve

@jetstack-bot jetstack-bot added the lgtm Indicates that a PR is ready to be merged. label Jan 11, 2024
@jetstack-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: maelvls

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

@jetstack-bot jetstack-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 11, 2024
@jetstack-bot jetstack-bot merged commit aba4393 into cert-manager:master Jan 11, 2024
8 checks passed
@wallrj wallrj deleted the format-issuers-table branch January 11, 2024 10:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants