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

docs: updated to include a new way to break down start/stop #10

Merged
merged 3 commits into from
Jun 26, 2024

Conversation

ethanholz
Copy link
Contributor

@ethanholz ethanholz commented Jun 18, 2024

This PR aims to improve upon the lack of clear guidance on what is required for "start" vs "stop" actions related to #8.

@ethanholz ethanholz changed the title docs: updated to include a new way to breakdown start/stop docs: updated to include a new way to break down start/stop Jun 18, 2024
Copy link

codecov bot commented Jun 18, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.11%. Comparing base (24caa5b) to head (967c9a6).

Additional details and impacted files
@@           Coverage Diff           @@
##             main      #10   +/-   ##
=======================================
  Coverage   94.11%   94.11%           
=======================================
  Files           3        3           
  Lines         289      289           
=======================================
  Hits          272      272           
  Misses         17       17           
Flag Coverage Δ
unittests 94.11% <ø> (ø)

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.

@ethanholz
Copy link
Contributor Author

@dwhswenson looking for guidance on if this is a bit clearer of an image around what is uses in which action. Open to improvements!

Copy link
Contributor

@dwhswenson dwhswenson left a comment

Choose a reason for hiding this comment

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

Would it make sense to change these such that the "required" columns under start/stop became "required for start" / "required for stop"? There are things marked as not required, which, now that the sections are nicely separated like this, might be misleading. (Basically anything that says, e.g., "will not start if not specified")

Similarly, there could be some clarification that the instance_mapping is the same as the output generated by the start process, and a clarification under Outputs that this is only an output from the start process.

A few changes, but I like this overall approach to make it clearer. I think this is already an improvement.

@ethanholz
Copy link
Contributor Author

Agreed, more than happy to add these additions for clarity!

The README table now includes info on the defaults as well as updates to
AWS defaults
@ethanholz
Copy link
Contributor Author

ethanholz commented Jun 25, 2024

Added some new areas for clarity. The CI probably shouldn't update unless we are actively updating code, codecov and pytest runs are not necessary when updating docs alone.

@dwhswenson this is ready for re-review.

Copy link
Contributor

@dwhswenson dwhswenson left a comment

Choose a reason for hiding this comment

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

LGTM!

@dwhswenson dwhswenson merged commit 81c4f29 into omsf-eco-infra:main Jun 26, 2024
5 checks passed
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.

2 participants