Skip to content

Improve vsphere doc #736

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
Merged
merged 1 commit into from
Apr 7, 2020
Merged

Improve vsphere doc #736

merged 1 commit into from
Apr 7, 2020

Conversation

@irozzo-1A
Copy link
Contributor

@irozzo-1A irozzo-1A commented Apr 3, 2020

What this PR does / why we need it:

Which issue(s) this PR fixes (optional, in fixes #<issue number> format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:

Check that links are not broken and how it's rendered here

Optional Release Note:

NONE

@kubermatic-bot kubermatic-bot added release-note-none Denotes a PR that doesn't merit a release note. dco-signoff: yes Denotes that all commits in the pull request have the valid DCO signoff message. labels Apr 3, 2020
@kubermatic-bot
Copy link
Contributor

kubermatic-bot commented Apr 3, 2020

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: irozzo-1A

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

@kubermatic-bot kubermatic-bot added sig/cluster-management Denotes a PR or issue as being assigned to SIG Cluster Management. approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 3, 2020
@irozzo-1A irozzo-1A force-pushed the improve-vsphere-doc branch 2 times, most recently from 5a38aae to c2279c6 Compare April 6, 2020 09:24
@irozzo-1A
Copy link
Contributor Author

irozzo-1A commented Apr 6, 2020

/retest

kron4eg
kron4eg requested changes Apr 6, 2020
View reviewed changes
Copy link
Member

@kron4eg kron4eg left a comment

Choose a reason for hiding this comment

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

I think there should be some consistency with code blocks. Ether indentation or backticks should be used to indicate codeblock, not mix of them

@irozzo-1A
Copy link
Contributor Author

irozzo-1A commented Apr 6, 2020

I think there should be some consistency with code blocks. Ether indentation or backticks should be used to indicate codeblock, not mix of them

@kron4eg I'm consistently using back-ticks for codeblocks, the indentation you mentioned is because it's inside a numbered list

@irozzo-1A irozzo-1A force-pushed the improve-vsphere-doc branch from c2279c6 to 97512e0 Compare April 7, 2020 07:54
kron4eg
kron4eg reviewed Apr 7, 2020
View reviewed changes
Copy link
Member

@kron4eg kron4eg left a comment

Choose a reason for hiding this comment

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

@irozzo-1A here are examples of inconsistency

docs/vsphere.md
VSphere provider accepts the following configuration parameters:
```yaml
Copy link
Member

@kron4eg kron4eg Apr 7, 2020

Choose a reason for hiding this comment

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

  • backticks
  • syntax hint
  • no indent

Copy link
Contributor Author

@irozzo-1A irozzo-1A Apr 7, 2020

Choose a reason for hiding this comment

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

no indent because we are not inside a numbered list

docs/vsphere.md

1. Download the `OVA` for the targeted OS.

```
Copy link
Member

@kron4eg kron4eg Apr 7, 2020

Choose a reason for hiding this comment

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

  • backticks
  • no syntax hint
  • indent

plus bunch of same blocks

Copy link
Contributor Author

@irozzo-1A irozzo-1A Apr 7, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

  • backticks

this is consistently used in the whole document

  • no syntax hint

no synthax hint because those blocks just contain some commands, I could put bash but does not provide much value as it is not something structured like json, yaml, etc.

  • indent

because we are inside a numbered list

plus bunch of same blocks

The same apply for all other blocks

@kubermatic-bot kubermatic-bot added the lgtm Indicates that a PR is ready to be merged. label Apr 7, 2020
@kubermatic-bot
Copy link
Contributor

kubermatic-bot commented Apr 7, 2020

LGTM label has been added.

Git tree hash: f747801fbc156fca1ad36cd95d60512b4624cfb5

@kubermatic-triage-bot
Copy link

kubermatic-triage-bot commented Apr 7, 2020

/retest
This bot automatically retries jobs that failed/flaked on approved PRs

Review the full test history

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

Also, here is a cat.
/meow

@kubermatic-bot
Copy link
Contributor

kubermatic-bot commented Apr 7, 2020

@kubermatic-triage-bot: cat image

In response to this:

/retest
This bot automatically retries jobs that failed/flaked on approved PRs

Review the full test history

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

Also, here is a cat.
/meow

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@kubermatic-bot kubermatic-bot merged commit ea01ca0 into kubermatic:master Apr 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kron4eg kron4eg kron4eg approved these changes

Assignees

@kron4eg kron4eg

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Denotes that all commits in the pull request have the valid DCO signoff message. lgtm Indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. sig/cluster-management Denotes a PR or issue as being assigned to SIG Cluster Management. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@irozzo-1A @kubermatic-bot @kubermatic-triage-bot @kron4eg