Skip to content

add commands explanation #26800

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

Closed
AgnesNM wants to merge 1 commit into from
Closed

add commands explanation #26800

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions docs/helm-chart/index.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,10 @@ To install this chart using Helm 3, run the following commands:
helm repo add apache-airflow https://airflow.apache.org
helm upgrade --install airflow apache-airflow/airflow --namespace airflow --create-namespace

**Explanation**:
`helm repo add apache-airflow https://airflow.apache.org`
- `helm repo add` is a command to add a chart repository

Comment on lines +93 to +96
Copy link
Contributor

@dstandish dstandish Oct 3, 2022

Choose a reason for hiding this comment

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

Hey @AgnesNM thanks for your contribution.

I think it might be better rework the existing instructions for clarity rather than adding an explanation block.

So for example, replace this:

To install this chart using Helm 3, run the following commands:
.. code-block:: bash
    helm repo add apache-airflow https://airflow.apache.org
    helm upgrade --install airflow apache-airflow/airflow --namespace airflow --create-namespace

With

To install this chart using Helm 3, first add the helm chart repository:

.. code-block:: bash
    helm repo add apache-airflow https://airflow.apache.org

Next, install the helm chart:

.. code-block:: bash
    helm upgrade --install airflow apache-airflow/airflow --namespace airflow --create-namespace

That way, we get the description saying what these commands actually do. Better continuity compared with explaining in a diff section.

The command deploys Airflow on the Kubernetes cluster in the default configuration. The :doc:`parameters-ref`
section lists the parameters that can be configured during installation.

Expand All @@ -105,6 +109,13 @@ To upgrade the chart with the release name ``airflow``:

helm upgrade airflow apache-airflow/airflow --namespace airflow

**Explanation**
- `helm upgrade` is a command to upgrade a release
- the first 'airflow' argument is the release name
- 'apache-airflow/airflow' is the Helm Chart
- the second 'airflow' argument is the namespace


Comment on lines +112 to +118
Copy link
Contributor

@dstandish dstandish Oct 3, 2022

Choose a reason for hiding this comment

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

similar to above, i would suggest that you amend existing wording if not clear already rather than adding an explanation section.

I would not document the argument syntax --- that's not our role in airflow, that is the job of the helm docs.

.. note::
To upgrade to a new version of the chart, run ``helm repo update`` first.

Expand All @@ -117,6 +128,12 @@ To uninstall/delete the ``airflow`` deployment:

helm delete airflow --namespace airflow

**Explanation**
- `helm delete` is a command to delete a release (it was renamed to `helm uninstall`)
- the first 'airflow' argument is the release name
- the second 'airflow' argument is the namespace


Comment on lines +131 to +136
Copy link
Contributor

@dstandish dstandish Oct 3, 2022

Choose a reason for hiding this comment

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

instead, can you amend existing to replace delete with uninstall?

also, rather than explaining this

- the first 'airflow' argument is the release name
- the second 'airflow' argument is the namespace

maybe better would be to choose better names in the example in the first place... so instead of airflow (namespace) choose my-namespace, and instead of airflow (release) maybe choose my-release-name or my-airflow-release.... WDYT?

Copy link
Contributor

@blag blag Oct 3, 2022

Choose a reason for hiding this comment

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

I like this idea. It helps break up the Helm commands to identify different things with different names, instead of leaving that implicit. I would try to identify things with their types, so something like my-airflow-namespace, even though that looks a little wordy, it's more helpful for people who aren't familiar with it.

Copy link
Contributor

@blag blag Oct 3, 2022

Choose a reason for hiding this comment

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

Another thing that might help is using (making up a term here) the "options assignment" syntax, if Helm supports it:

helm repo add apache-airflow https://airflow.apache.org
helm upgrade --install=airflow apache-airflow/airflow --create-namespace --namespace=airflow
helm upgrade airflow apache-airflow/airflow --namespace=airflow
helm delete airflow --namespace=airflow

But if it doesn't then feel free to disregard.

Copy link
Contributor

@dstandish dstandish Oct 3, 2022

Choose a reason for hiding this comment

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

Yeah that's a good idea @blag but unfortunately I don't think that the helm command allows that... it's just positional

I think I found this confusing at one point in the past also

The command removes all the Kubernetes components associated with the chart and deletes the release.

.. note::
Expand Down