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

helm: improvement in docs #12734

Merged
merged 2 commits into from
Aug 21, 2023
Merged

helm: improvement in docs #12734

merged 2 commits into from
Aug 21, 2023

Conversation

parth-gr
Copy link
Member

Description of your changes:

values.overide isn't used for cephcluster
we use values.yaml so updated the doc with the
change

Which issue is resolved by this Pull Request:
Resolves #

Checklist:

  • Commit Message Formatting: Commit titles and messages follow guidelines in the developer guide.
  • Skip Tests for Docs: If this is only a documentation change, add the label skip-ci on the PR.
  • Reviewed the developer guide on Submitting a Pull Request
  • Pending release notes updated with breaking and/or notable changes for the next minor release.
  • Documentation has been updated, if necessary.
  • Unit tests have been added, if necessary.
  • Integration tests have been added, if necessary.

@parth-gr parth-gr requested a review from travisn August 16, 2023 08:20
@parth-gr parth-gr force-pushed the helm-improvements branch 2 times, most recently from 808f376 to c076dfd Compare August 16, 2023 08:30
Documentation/Helm-Charts/ceph-cluster-chart.gotmpl.md Outdated Show resolved Hide resolved
Documentation/Helm-Charts/ceph-cluster-chart.gotmpl.md Outdated Show resolved Hide resolved
@@ -36,6 +36,9 @@ helm repo add rook-release https://charts.rook.io/release
helm install --create-namespace --namespace rook-ceph rook-ceph rook-release/rook-ceph -f values.yaml
```

!!! Note
--namespace specify the operator namespace.
Copy link
Member

Choose a reason for hiding this comment

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

Do you think this note is necessary? I don't see what other namespace it would be confused with, or that needs clarifying. The --namespace flag is a common helm flag, so it seems ok not to explain more.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes right the --namespace is provided by helm,
But first time I really get confused as we generally use namespace as cephcluster namespace,
I am fine keeping it or not keeping it, as per your suggestion.

Copy link
Member

Choose a reason for hiding this comment

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

I vote to remove this note in the operator helm chart doc. We can keep the clarification in the other doc since the cluster helm chart has both the cluster namespace and operator namespace.

@@ -7,7 +7,7 @@ image:
repository: rook/ceph
# -- Image tag
# @default -- `master`
tag: VERSION
Copy link
Member

Choose a reason for hiding this comment

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

The helm build replaces this version when it publishes the helm chart, so I don't think we can change this.

Copy link
Member Author

@parth-gr parth-gr Aug 17, 2023

Choose a reason for hiding this comment

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

I just make the build change,
Lemme know if it's feels good, otherwise, I can revert the changes

Copy link
Member

Choose a reason for hiding this comment

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

I believe this will work fine since the makefile is updated.

  1. You're making this change so it's easier to test the local helm chart with the master tag, right?
  2. Please create a new commit for this tag change. It's not just a doc improvement like the rest.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes exactly,
Added it as separate commit
Thanks

@@ -36,6 +36,9 @@ helm repo add rook-release https://charts.rook.io/release
helm install --create-namespace --namespace rook-ceph rook-ceph rook-release/rook-ceph -f values.yaml
```

!!! Note
--namespace specify the operator namespace.
Copy link
Member

Choose a reason for hiding this comment

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

I vote to remove this note in the operator helm chart doc. We can keep the clarification in the other doc since the cluster helm chart has both the cluster namespace and operator namespace.

@@ -7,7 +7,7 @@ image:
repository: rook/ceph
# -- Image tag
# @default -- `master`
tag: VERSION
Copy link
Member

Choose a reason for hiding this comment

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

I believe this will work fine since the makefile is updated.

  1. You're making this change so it's easier to test the local helm chart with the master tag, right?
  2. Please create a new commit for this tag change. It's not just a doc improvement like the rest.

values.overide isn't used for cephcluster
we use values.yaml so updated the doc with the
change

Signed-off-by: parth-gr <paarora@redhat.com>
updating this to ease the local testing
using helm for master branch repo

Signed-off-by: parth-gr <paarora@redhat.com>
@travisn travisn merged commit 9a5ea4c into rook:master Aug 21, 2023
45 of 49 checks passed
travisn added a commit that referenced this pull request Aug 21, 2023
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.

None yet

2 participants