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

chore: attempt to improve documentation #496

Merged

Conversation

Kavindu-Dodan
Copy link
Contributor

@Kavindu-Dodan Kavindu-Dodan commented Jul 5, 2023

This PR

Attempts to improve OFO documentation

Note - this should fix #483

@Kavindu-Dodan Kavindu-Dodan requested a review from a team as a code owner July 5, 2023 22:55
@codecov
Copy link

codecov bot commented Jul 5, 2023

Codecov Report

Merging #496 (4c4437a) into main (1ec3183) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #496   +/-   ##
=======================================
  Coverage   82.53%   82.53%           
=======================================
  Files          22       22           
  Lines        1386     1386           
=======================================
  Hits         1144     1144           
  Misses        205      205           
  Partials       37       37           
Flag Coverage Δ
unit-tests 82.53% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Copy link
Contributor

@agardnerIT agardnerIT left a comment

Choose a reason for hiding this comment

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

Thanks for these changes. They largely look great! A few comments though:

  1. docs/getting_started.md and docs/quick_start.md should be combined. They serve largely the same purpose and as such, having both is confusing and redundant.
  2. At the end of docs/installation.md we should link to docs/getting_started.md otherwise, after installation, I don't know what else to do.
  3. At the end of docs/quick_start.md (or wherever that content ends up, we should mention that the OFO uses flagd as a flag provider and thus all the flagd documentation is also relevant. One specific callout is to point of the fact that ResolveString isn't the only option, but other options are available.

@Kavindu-Dodan Kavindu-Dodan changed the title attempt to improve documentation chore: attempt to improve documentation Jul 6, 2023
Signed-off-by: Kavindu Dodanduwa <kavindudodanduwa@gmail.com>
Signed-off-by: Kavindu Dodanduwa <kavindudodanduwa@gmail.com>
@Kavindu-Dodan
Copy link
Contributor Author

Thanks for these changes. They largely look great! A few comments though:

  1. docs/getting_started.md and docs/quick_start.md should be combined. They serve largely the same purpose and as such, having both is confusing and redundant.
  2. At the end of docs/installation.md we should link to docs/getting_started.md otherwise, after installation, I don't know what else to do.
  3. At the end of docs/quick_start.md (or wherever that content ends up, we should mention that the OFO uses flagd as a flag provider and thus all the flagd documentation is also relevant. One specific callout is to point of the fact that ResolveString isn't the only option, but other options are available.

thank you @agardnerIT for the feedback. I addressed all your points

  1. Kept one quick start guide : this is now comprehensive and includes minimal installation instructions
  2. Added the link to quick start guide : operator installation can be skipped but users can follow other steps to deploy a workload
  3. Added the what's next section [1] linking to other resources

[1] - https://github.com/open-feature/open-feature-operator/pull/496/files#diff-61f4196322cfa9f52c5f548f13b7f781243b2cae898862a8688462d914ba7173R178-R182

docs/README.md Outdated Show resolved Hide resolved
Copy link
Member

@toddbaert toddbaert left a comment

Choose a reason for hiding this comment

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

This is an improvement, but I think there's a mistake here.

@Kavindu-Dodan
Copy link
Contributor Author

This is an improvement, but I think there's a mistake here.

Thank you. I corrected them and also expanded Flag Source configuration section [1] with other flag sources and configuration options. Strangely, we never documented all the options we supported

[1] - https://github.com/open-feature/open-feature-operator/blob/b7a6b531ed9ab6edfe5ecf6038bfdc2ae570a811/docs/flag_source_configuration.md

Signed-off-by: Kavindu Dodanduwa <kavindudodanduwa@gmail.com>
Signed-off-by: Kavindu Dodanduwa <kavindudodanduwa@gmail.com>
docs/README.md Outdated Show resolved Hide resolved
Copy link
Member

@toddbaert toddbaert left a comment

Choose a reason for hiding this comment

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

LGTM, but I'd like to see this last small change: 1dbf3d0#r1258239251

Kavindu-Dodan and others added 2 commits July 10, 2023 09:27
Co-authored-by: Giovanni Liva <giovanni.liva@dynatrace.com>
Signed-off-by: Kavindu Dodanduwa <Kavindu-Dodan@users.noreply.github.com>
Signed-off-by: Kavindu Dodanduwa <kavindudodanduwa@gmail.com>
@toddbaert toddbaert self-requested a review July 10, 2023 18:26
Copy link
Member

@toddbaert toddbaert left a comment

Choose a reason for hiding this comment

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

@Kavindu-Dodan the last set of changes looks great! I think after it's merged we could put @thisthat 's changes in/around these changes as a sub-point.

Signed-off-by: Kavindu Dodanduwa <kavindudodanduwa@gmail.com>
docs/README.md Outdated Show resolved Hide resolved
docs/README.md Outdated Show resolved Hide resolved
docs/README.md Outdated Show resolved Hide resolved
docs/annotations.md Outdated Show resolved Hide resolved
docs/concepts.md Outdated Show resolved Hide resolved
docs/flagd_proxy.md Outdated Show resolved Hide resolved
docs/flagd_proxy.md Outdated Show resolved Hide resolved
docs/quick_start.md Outdated Show resolved Hide resolved
docs/quick_start.md Outdated Show resolved Hide resolved
docs/quick_start.md Outdated Show resolved Hide resolved
Kavindu-Dodan and others added 3 commits July 10, 2023 11:38
Co-authored-by: Michael Beemer <beeme1mr@users.noreply.github.com>
Signed-off-by: Kavindu Dodanduwa <Kavindu-Dodan@users.noreply.github.com>
Co-authored-by: Michael Beemer <beeme1mr@users.noreply.github.com>
Signed-off-by: Kavindu Dodanduwa <Kavindu-Dodan@users.noreply.github.com>
Co-authored-by: Michael Beemer <beeme1mr@users.noreply.github.com>
Signed-off-by: Kavindu Dodanduwa <Kavindu-Dodan@users.noreply.github.com>
Kavindu-Dodan and others added 9 commits July 10, 2023 11:44
Signed-off-by: Kavindu Dodanduwa <kavindudodanduwa@gmail.com>
Co-authored-by: Michael Beemer <beeme1mr@users.noreply.github.com>
Signed-off-by: Kavindu Dodanduwa <Kavindu-Dodan@users.noreply.github.com>
Co-authored-by: Michael Beemer <beeme1mr@users.noreply.github.com>
Signed-off-by: Kavindu Dodanduwa <Kavindu-Dodan@users.noreply.github.com>
Co-authored-by: Michael Beemer <beeme1mr@users.noreply.github.com>
Signed-off-by: Kavindu Dodanduwa <Kavindu-Dodan@users.noreply.github.com>
Co-authored-by: Michael Beemer <beeme1mr@users.noreply.github.com>
Signed-off-by: Kavindu Dodanduwa <Kavindu-Dodan@users.noreply.github.com>
Co-authored-by: Michael Beemer <beeme1mr@users.noreply.github.com>
Signed-off-by: Kavindu Dodanduwa <Kavindu-Dodan@users.noreply.github.com>
Co-authored-by: Michael Beemer <beeme1mr@users.noreply.github.com>
Signed-off-by: Kavindu Dodanduwa <Kavindu-Dodan@users.noreply.github.com>
Co-authored-by: Michael Beemer <beeme1mr@users.noreply.github.com>
Signed-off-by: Kavindu Dodanduwa <Kavindu-Dodan@users.noreply.github.com>
Co-authored-by: Michael Beemer <beeme1mr@users.noreply.github.com>
Signed-off-by: Kavindu Dodanduwa <Kavindu-Dodan@users.noreply.github.com>
docs/annotations.md Outdated Show resolved Hide resolved
Kavindu-Dodan and others added 2 commits July 10, 2023 12:34
Co-authored-by: Michael Beemer <beeme1mr@users.noreply.github.com>
Signed-off-by: Kavindu Dodanduwa <Kavindu-Dodan@users.noreply.github.com>
Co-authored-by: Michael Beemer <beeme1mr@users.noreply.github.com>
Signed-off-by: Kavindu Dodanduwa <Kavindu-Dodan@users.noreply.github.com>
@Kavindu-Dodan Kavindu-Dodan merged commit 603e74e into open-feature:main Jul 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[DOCS] Docs improvements and possible bug
5 participants