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

Docs: recommend to install from packages #285

Merged
merged 4 commits into from
Nov 28, 2022
Merged

Docs: recommend to install from packages #285

merged 4 commits into from
Nov 28, 2022

Conversation

Al2Klimov
Copy link
Member

@Al2Klimov Al2Klimov commented Sep 22, 2022

Copy link
Member

@bobapple bobapple left a comment

Choose a reason for hiding this comment

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

Looks good to me 👍

If you want to see it live in action, here's the rendered version: https://icinga.com/docs/icinga-web-graphite-integration/docs/

@Al2Klimov Al2Klimov added this to the v1.2.2 milestone Sep 27, 2022
Copy link
Member

@lippserd lippserd left a comment

Choose a reason for hiding this comment

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

In addition to the requested changes please omit the the in headings, e.g. Installing the ..., Configuring the .... This is also the case in our other docs.

```
<!-- {% endif %} -->

## Installing the Package
Copy link
Member

Choose a reason for hiding this comment

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

Our other docs mention the product, please do so here as well, i.e. Installing Icinga Web Graphite Integration Package.

Please follow the steps listed for your target operating system,
which guide you through setting up the repository and installation.

<!-- {% else %} -->
## Requirements
Copy link
Member

Choose a reason for hiding this comment

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

I would remove that completely, since it should be pretty obvious that you need Graphite to actually integrate it. 😆.

Copy link
Member Author

Choose a reason for hiding this comment

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

Graphite: yes. Graphite Web... ? I mean, I've grepped for Graphite Web and with a such change you'd have your Graphite doing... its Graphite things, you'll go through our install doc like oh, cool, I've installed everything and then in our config doc you'll be surprised with a Graphite Web URL being needed.

Copy link
Member

Choose a reason for hiding this comment

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

[...] oh, cool, I've installed everything and then in our config doc you'll be surprised with a Graphite Web URL being needed.

You are absolutely right.

@@ -1,30 +1,31 @@
# <a id="Demonstration"></a>Demonstration
Copy link
Member

Choose a reason for hiding this comment

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

#287 will remove the Dockerfile. Please revert the changes made here as this file is also subject to removal.

Copy link
Member Author

Choose a reason for hiding this comment

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

That issue shouldn’t take long to resolve. I'll rebase this one on top if necessary.

Copy link
Member

Choose a reason for hiding this comment

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

Ready to rebase.

README.md Outdated Show resolved Hide resolved
module.info Outdated Show resolved Hide resolved
Copy link
Member

@nilmerg nilmerg left a comment

Choose a reason for hiding this comment

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

I don't quite like that the from source section is now buried in the sub directory. The ones reading this on Github are more likely to need that section in the main installation section.

README.md Outdated Show resolved Hide resolved
@lippserd
Copy link
Member

I don't quite like that the from source section is now buried in the sub directory. The ones reading this on Github are more likely to need that section in the main installation section.

There is a link pointing right to it in installation.md.

@Al2Klimov
Copy link
Member Author

I'll first have a look if you can convince Eric to do the same for Cube.

@nilmerg
Copy link
Member

nilmerg commented Oct 27, 2022

I don't quite like that the from source section is now buried in the sub directory. The ones reading this on Github are more likely to need that section in the main installation section.

There is a link pointing right to it in installation.md.

Ah, didn't notice that!

Copy link
Member

@nilmerg nilmerg left a comment

Choose a reason for hiding this comment

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

I still find it wrong to read Configuring Icinga Web Graphite Integration and not Configuring the Icinga Web Graphite Integration. It's just better wording.

doc/02-Installation.md Outdated Show resolved Hide resolved
doc/02-Installation.md Outdated Show resolved Hide resolved
doc/02-Installation.md Outdated Show resolved Hide resolved
@lippserd lippserd merged commit dd1b2cf into master Nov 28, 2022
@lippserd lippserd deleted the docs branch November 28, 2022 08:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants