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

Improve installing on Minikube docs #1613

Merged
merged 2 commits into from
Sep 28, 2020
Merged

Improve installing on Minikube docs #1613

merged 2 commits into from
Sep 28, 2020

Conversation

mmorhun
Copy link
Contributor

@mmorhun mmorhun commented Sep 25, 2020

Signed-off-by: Mykola Morhun mmorhun@redhat.com

What does this PR do?

Fixes installation on Minikube guide

What issues does this PR fix or reference?

eclipse-che/che#17952

Specify the version of the product this PR applies to.

Eclipse Che 7.x

PR Checklist

As the author of this Pull Request I made sure that:

  • vale has been run successfully against the PR branch
  • Link checker has been run successfully against the PR branch
  • Documentation describes a scenario that is already covered by QE tests, otherwise an issue has been created and acknowledged by Che QE team
  • Changed article references are updated where they are used (or a redirect has been set up on the docs side):

Signed-off-by: Mykola Morhun <mmorhun@redhat.com>
----
+
NOTE: For Windows and Mac users it is required to add the `--vm=true` option to run Minikube in a virtual machine.
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI notes are described with

[NOTE]
====
text here

Copy link
Contributor

Choose a reason for hiding this comment

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

did you run vale plug-in on the changes ?
you should see error on passive is required

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've seen the NOTE: definition as well in many places. Is there any rule for this?

I didn't run vale on my branch (as checklist says) because I have problems with workspace from default devfile.

@@ -14,11 +14,22 @@ This section describes how to install {prod-short} on Minikube using {prod-cli}.

.Procedure

* Run the following command:
* To install {prod-short} in multi-user mode (default and recommended), run the following command:
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a getting started, we should keep things simple. Since it's the default and recommended we should not go through much of the details. I would not change this line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, I'll revert this line

+
NOTE: Omit the `--multiuser` option to install a single-user instance of {prod-short}.
Copy link
Contributor

Choose a reason for hiding this comment

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

To keep things simple can we just say:

NOTE: Add option `--installer helm` to use the helm chart and install a single-user instance of {prod-short}.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

@@ -17,5 +17,7 @@ This section describes how to use Minikube to set up a local single-node {kubern
. Start Minikube (it is important to *allocate at least 4 GB of RAM*):
+
----
$ minikube start --memory=4096
$ minikube start --addons=ingress --memory=4096
Copy link
Contributor

Choose a reason for hiding this comment

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

On linux isn't --vm=true the default?
If that's the case we could start using the same command on all platforms:

minikube start --addons=ingress --memory=4096 --vm=true

Copy link
Contributor

Choose a reason for hiding this comment

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

Another doubt: is 4G enough to run Che multi-user, start Quarkus stack, compile and run it? I am afraid that this is not enough anymore. If that's the case I would change to "at least 4GB of RAM but 8GB are recommended" and use --memory=8192 in the command line.

Copy link
Contributor Author

@mmorhun mmorhun Sep 25, 2020

Choose a reason for hiding this comment

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

On Linux option --vm=true is not required. And from resource usage docker driver behaves better.
But if you think that we should include --vm=true as default, I may add it.

Copy link
Contributor Author

@mmorhun mmorhun Sep 25, 2020

Choose a reason for hiding this comment

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

Agree, the memory amount is very minimal.

@mmorhun mmorhun force-pushed the che-17952 branch 2 times, most recently from 761064f to dbeba97 Compare September 25, 2020 09:21
Signed-off-by: Mykola Morhun <mmorhun@redhat.com>
@mmorhun
Copy link
Contributor Author

mmorhun commented Sep 25, 2020

In this PR vale checks failed with weird errors. However, in my workspace I do not see them:
Screenshot from 2020-09-25 12-39-53
What should I do? @themr0c @rkratky @MichalMaler

@mmorhun mmorhun marked this pull request as ready for review September 25, 2020 11:09
@themr0c
Copy link
Contributor

themr0c commented Sep 25, 2020

It seems 2 things are happening:

  • The vale-pr-check is running with the latest changes in vale config (2 styles only: CheDocs and IBM), and the workspace is running with older config (more style names). (PR merged yesterday, the checks are the same, only the display is adapted). No worry for that.
  • The workspace doesn't display results for the Terms checks -> they were added yesterday. We need improvement on pattern matching. Ignore them for now.
  • The workspace doesn't display results for the Spelling checks -> they need vale >= 2.3 => we need to check the version of vale the sidecar container. Please ignore these results for this PR.
    I apologize for these inconsistencies.

@mmorhun mmorhun merged commit e5493c8 into master Sep 28, 2020
@mmorhun mmorhun deleted the che-17952 branch September 28, 2020 07:25
@che-bot che-bot added this to the 7.20 milestone Sep 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
5 participants