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

Simmy docs #1883

Merged
merged 38 commits into from
Jan 19, 2024
Merged

Simmy docs #1883

merged 38 commits into from
Jan 19, 2024

Conversation

vany0114
Copy link
Contributor

@vany0114 vany0114 commented Jan 8, 2024

Simmy docs.

Didn't run the docs website locally bc I don't know how, if you guys think I should please let me know how.

# Conflicts:
#	src/Polly.Core/PublicAPI.Unshipped.txt
* add logos
* add Simmy basic docs
* add chaos engineering menu and its options
* fix fault options docs
* add fault strategy docs
* document common options across chaos strategies
* remove chaos engineering option from the advanced menu
@peter-csala
Copy link
Contributor

Simmy docs.

Didn't run the docs website locally bc I don't know how, if you guys think I should please let me know how.

You just have to run these two commands and you can test the documentation website locally:

Download the docfx among other tools

dotnet tool restore

Build and run the website

dotnet docfx docs/docfx.json --serve

visit the http://localhost:8080 url

@peter-csala
Copy link
Contributor

@vany0114 Could you please update the PR description in a way that you highlight what is already done and what's still missing? Then we can split the remaining work.

Copy link
Member

@martincostello martincostello left a comment

Choose a reason for hiding this comment

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

Some words need adding to the spellcheck dictionary and the C# snippets need to be added as code.

docs/chaos/behavior.md Outdated Show resolved Hide resolved
docs/chaos/behavior.md Outdated Show resolved Hide resolved
docs/chaos/behavior.md Outdated Show resolved Hide resolved
docs/chaos/fault.md Outdated Show resolved Hide resolved
docs/chaos/fault.md Outdated Show resolved Hide resolved
docs/chaos/latency.md Outdated Show resolved Hide resolved
docs/chaos/latency.md Outdated Show resolved Hide resolved
docs/chaos/result.md Show resolved Hide resolved
docs/chaos/result.md Outdated Show resolved Hide resolved
docs/chaos/result.md Outdated Show resolved Hide resolved
@martincostello martincostello added this to the v8.3.0 milestone Jan 8, 2024
@peter-csala
Copy link
Contributor

peter-csala commented Jan 8, 2024

@vany0114 Please allow me (@peter-csala) to contribute to your branch (otherwise I have to create a fork from your branch).

@vany0114
Copy link
Contributor Author

@vany0114 Please allow me (@peter-csala) to contribute to your branch (otherwise I have to create a fork from your branch).

@peter-csala done

src/Snippets/Docs/Chaos.Behavior.cs Outdated Show resolved Hide resolved
src/Snippets/Docs/Chaos.Behavior.cs Outdated Show resolved Hide resolved
src/Snippets/Docs/Chaos.Behavior.cs Outdated Show resolved Hide resolved
src/Snippets/Docs/Chaos.Fault.cs Outdated Show resolved Hide resolved
src/Snippets/Docs/Chaos.Fault.cs Outdated Show resolved Hide resolved
src/Snippets/Docs/Chaos.Latency.cs Outdated Show resolved Hide resolved
src/Snippets/Docs/Chaos.Latency.cs Outdated Show resolved Hide resolved
src/Snippets/Docs/Chaos.Result.cs Outdated Show resolved Hide resolved
src/Snippets/Docs/Chaos.Result.cs Outdated Show resolved Hide resolved
docs/chaos/index.md Outdated Show resolved Hide resolved
@martintmk
Copy link
Contributor

Added some comments, otherwise it looks really great @vany0114 and @peter-csala!

Copy link

codecov bot commented Jan 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (dc8248d) 84.65% compared to head (a2f0986) 84.65%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1883   +/-   ##
=======================================
  Coverage   84.65%   84.65%           
=======================================
  Files         309      309           
  Lines        6828     6828           
  Branches     1050     1050           
=======================================
  Hits         5780     5780           
  Misses        839      839           
  Partials      209      209           
Flag Coverage Δ
linux 84.65% <ø> (ø)
macos 84.65% <ø> (ø)
windows 84.65% <ø> (ø)

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@peter-csala
Copy link
Contributor

@vany0114 When I was reading the chaos engineering page I was missing further resources links. Since the page talks mainly about simmy not about chaos engineering I think it might make sense to add links which are explaining the fundamentals (like hypothesis, steady-state, experiment, blast radius, etc.)

Copy link
Contributor

@martintmk martintmk left a comment

Choose a reason for hiding this comment

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

Few nits, otherwise LGTM

@martintmk
Copy link
Contributor

Let's get rid of extra logos and :shipit:.

Wdyt, @martincostello anything else is missing?

@martincostello
Copy link
Member

I have three unresolved comments, but otherwise I'm happy if you and @vany0114 are happy.

docs/chaos/index.md Outdated Show resolved Hide resolved
docs/chaos/index.md Outdated Show resolved Hide resolved
@peter-csala
Copy link
Contributor

@martincostello , @martintmk For me it seems a bit odd to have Chaos engineering section between Resilience strategies and Resilience pipelines. What do you guys think where should we place it inside the TOC?

Screenshot 2024-01-18 at 10 29 35

@martincostello
Copy link
Member

Let's put it just before the advanced section.

@peter-csala
Copy link
Contributor

Is there anything that needs to be addressed or can we :shipit: ?

@martintmk
Copy link
Contributor

Is there anything that needs to be addressed or can we :shipit: ?

I think it fine to merge. @martincostello ?

@martincostello
Copy link
Member

Just waiting on some feedback from @vany0114.

@vany0114
Copy link
Contributor Author

@peter-csala thanks a lot for the help! it looks good to me.

@martincostello martincostello merged commit 9410d22 into App-vNext:main Jan 19, 2024
17 checks passed
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.

4 participants