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

Add docs #1

Merged
merged 8 commits into from
Sep 6, 2021
Merged

Add docs #1

merged 8 commits into from
Sep 6, 2021

Conversation

GeorgianaElena
Copy link
Member

@GeorgianaElena GeorgianaElena commented Sep 2, 2021

This puts together some of the docs we already have.

Reference: 2i2c-org/infrastructure#631

@choldgraf
Copy link
Member

hey @GeorgianaElena - would you like 👀 or still want to finish stuff up first? The PR is not in "draft mode", but the title says "WIP" so I am not sure which to believe :-)

Copy link
Member

@choldgraf choldgraf left a comment

Choose a reason for hiding this comment

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

This looks great to me - just need to add in the images etc and I think it is a great start. We should ask someone to try out the instructions and see if it works!

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@GeorgianaElena GeorgianaElena changed the title [WIP] Add docs Add docs Sep 3, 2021
@GeorgianaElena
Copy link
Member Author

Thanks @choldgraf! I added the screenshots now too.

What do you think if instead of tacking in this PR this:

Document a more complex setup too (with a Dockerfile), and give an example repo

I open an issue in this repo and tackle this task in another PR?

Copy link
Member

@sgibson91 sgibson91 left a comment

Choose a reason for hiding this comment

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

I have a few suggestions to clarify the text a little bit, but this looks great!!

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@sgibson91
Copy link
Member

Document a more complex setup too (with a Dockerfile), and give an example repo

I open an issue in this repo and tackle this task in another PR?

+1 but I think it would be useful to have another template repo that looks like https://github.com/2i2c-org/coessing-image for Pangeo-based images

Copy link
Member

@choldgraf choldgraf left a comment

Choose a reason for hiding this comment

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

This looks good to me after @sgibson91's suggestions are resolved. Thanks @GeorgianaElena for working this out! I think it will be very helpful for folks who want that extra control over their environment.

I've opened up #3 to discuss the more advanced use-cases and Dockerfiles. Perhaps we can take conversation about that over there? @sgibson91 one thing I am trying to understand is when you really need a dockerfile vs when you can get away with postBuild scripts and such. It would be helpful for us to converge a bit on best-practices there.

Copy link
Contributor

@damianavila damianavila left a comment

Choose a reason for hiding this comment

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

LGTM (modulo @sgibson91 comments and suggestions are addressed).

GeorgianaElena and others added 4 commits September 4, 2021 10:04
Co-authored-by: Sarah Gibson <[email protected]>
Co-authored-by: Sarah Gibson <[email protected]>
Co-authored-by: Sarah Gibson <[email protected]>
Co-authored-by: Sarah Gibson <[email protected]>
@GeorgianaElena
Copy link
Member Author

Thanks everyone! ☀️

@choldgraf
Copy link
Member

thanks for this great work @GeorgianaElena :-)

@GeorgianaElena GeorgianaElena deleted the add-readme branch September 7, 2021 09:13
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.

4 participants