Skip to content
This repository has been archived by the owner on Jul 11, 2023. It is now read-only.

doc(chart): Updated chart readme #2322

Merged
merged 7 commits into from
Jan 20, 2021
Merged

doc(chart): Updated chart readme #2322

merged 7 commits into from
Jan 20, 2021

Conversation

ams0
Copy link
Contributor

@ams0 ams0 commented Jan 19, 2021

Description:

A more descriptive Readme for the Helm chart

Affected area:

  • New Functionality [ ]
  • Documentation [X]
  • Install [ ]
  • Control Plane [ ]
  • CLI Tool [ ]
  • Certificate Management [ ]
  • Networking [ ]
  • Metrics [ ]
  • SMI Policy [ ]
  • Security [ ]
  • Tests [ ]
  • CI System [ ]
  • Performance [ ]
  • Other [ ]

Please answer the following questions with yes/no.

  • Does this change contain code from or inspired by another project? If so, did you notify the maintainers and provide attribution?

No

@ams0 ams0 requested a review from a team as a code owner January 19, 2021 23:35
Signed-off-by: Alessandro Vozza <alessandro.vozza@microsoft.com>
@ams0
Copy link
Contributor Author

ams0 commented Jan 19, 2021

@nojnhuh I know correctly entered the comments in the values.yaml and let helm-docs generate the readme. I did not actually included the readme as I assume the CI will take care of it?

@codecov-io
Copy link

codecov-io commented Jan 19, 2021

Codecov Report

Merging #2322 (37429ff) into main (db184fc) will decrease coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2322      +/-   ##
==========================================
- Coverage   57.25%   57.23%   -0.02%     
==========================================
  Files         149      149              
  Lines        6660     6660              
==========================================
- Hits         3813     3812       -1     
- Misses       2842     2843       +1     
  Partials        5        5              
Impacted Files Coverage Δ
pkg/envoy/route/config.go 95.23% <0.00%> (-0.80%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update db184fc...37429ff. Read the comment docs.

Copy link
Contributor

@michelleN michelleN left a comment

Choose a reason for hiding this comment

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

Thank you @ams0 ! These changes look great. Some of the comments seem like they are duplicates of the line above it. If you could remove the lines above that are duplicates, that would be great. Will merge as soon as that is done.

charts/osm/values.yaml Show resolved Hide resolved
charts/osm/values.yaml Show resolved Hide resolved
Signed-off-by: Alessandro Vozza <alessandro.vozza@microsoft.com>
@ams0
Copy link
Contributor Author

ams0 commented Jan 20, 2021

thanks @michelleN ! Also, to be addressed perhaps later, all values are prefixed with OpenServiceMesh, perhaps rendering it at all superfluous.

Copy link
Contributor

@michelleN michelleN left a comment

Choose a reason for hiding this comment

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

Had a minor question about the README but I'll go ahead and merge this and we can address the install line in a follow up PR.

## Install Chart

```console
helm install [RELEASE_NAME] osm/osm
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be charts/osm?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it seems that's how it looks in the chart repo:

~> helm search repo osm
NAME   	CHART VERSION	APP VERSION	DESCRIPTION
osm/osm	0.6.1        	v0.6.1     	A Helm chart to install the OSM control plane o...

Copy link
Contributor

@nojnhuh nojnhuh left a comment

Choose a reason for hiding this comment

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

@nojnhuh I know correctly entered the comments in the values.yaml and let helm-docs generate the readme. I did not actually included the readme as I assume the CI will take care of it?

We actually don't have this automated in CI currently, but we have it documented as a manual step as part of cutting releases. I'm not too worried about everything being perfectly in sync between releases. Thanks for digging in to figure out the helm-docs details.

@ksubrmnn ksubrmnn changed the title Updated chart readme ref(chart): Updated chart readme Jan 20, 2021
@ksubrmnn ksubrmnn changed the title ref(chart): Updated chart readme doc(chart): Updated chart readme Jan 20, 2021
@ksubrmnn ksubrmnn merged commit 85b5217 into openservicemesh:main Jan 20, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants