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: added docker instructions to the exercises #3523

Closed
wants to merge 10 commits into from
Closed

docs: added docker instructions to the exercises #3523

wants to merge 10 commits into from

Conversation

karl-cardenas-coding
Copy link

@karl-cardenas-coding karl-cardenas-coding commented Sep 25, 2022

Current Behavior

This PR addresses the challenges users may encounter when conducting the learning exercises in a local development environment. An example of an issue is #3521 , more examples can be found in the help thread.

Proposed Changes

The PR adds instructions on using a Docker container for the exercises featured in the main README.md. The user may still proceed with a local install should they choose to. The benefit is a better learning experience for the user and less time spent by the Ockam team on answering questions about a user's local environment, which can be difficult and time-consuming to debug.

There is a price to pay with this setup. The first compile session will take a few minutes due to updates to crates.io index.
In my opinion, the tradeoff of less time spent in helping users debug local environment issues makes this worth it for all parties.

Checks

@karl-cardenas-coding karl-cardenas-coding requested a review from a team as a code owner September 25, 2022 15:14
@karl-cardenas-coding karl-cardenas-coding marked this pull request as draft September 25, 2022 15:14
@karl-cardenas-coding karl-cardenas-coding changed the title docs: added docker instructions to the excersie in readme.md docs: added docker instructions to the exercises Sep 25, 2022
@karl-cardenas-coding karl-cardenas-coding marked this pull request as ready for review September 25, 2022 18:32
@karl-cardenas-coding
Copy link
Author

@mrinalwadhwa no rush on this PR. I went through all the exercises just to make sure I didn't miss anything, but I would appreciate a second glance 😄 Feel free to suggest changes or provide feedback. I have more ideas on how we can improve the learning, but I think just getting out of the debugging users' local environment will be a good win, to begin with.

@mrinalwadhwa
Copy link
Member

@karl-cardenas-coding thank you for spending time on this!
I'll read through the changes soon and leave suggestions if I have any.
Congratulations on an awesome first contribution 🥳

@mrinalwadhwa
Copy link
Member

A couple of quick things, if you have time to improve them.

  • The Editorconfig check is failing because some lines in the text you've added have trailing whitespace, if you could remove those, that would be great. https://github.com/build-trust/ockam/actions/runs/3123223435/jobs/5065789655

  • The Documentation check is also failing. This one checks to make sure that our examples in guides are not broken and are compilable. I suspect this failure is in the check code itself and its assumptions. Ignore this failure, I'll look into it when I have some more time to read the full PR.

Thank you

@mrinalwadhwa
Copy link
Member

removing trailing whitespace

Editors/IDEs usually have builtin commands that do this, more helpful information here https://editorconfig.org/

@karl-cardenas-coding
Copy link
Author

@mrinalwadhwa nice catch, I'll get that fixed tomorrow.

@karl-cardenas-coding
Copy link
Author

@mrinalwadhwa linting errors addressed ✔️

Copy link
Member

@adrianbenavides adrianbenavides left a comment

Choose a reason for hiding this comment

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

Hi @karl-cardenas-coding, there's been some recent PR's merged into develop which add some minor modifications to the README.md file that conflicts with your changes. Could you please add a commit to resolve the conflicts?

Thanks again for your time, your changes look great!

README.md Outdated
Upon completion, you will be placed inside the `/work` folder of the container. Next, add a text editior for editing files.

```
apt update && apt install nano
Copy link
Member

Choose a reason for hiding this comment

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

I didn't write the original guide but I would say that using touch to create the files and not specifying any tool to edit those files is intentional so the user can proceed with their preferred tool (everyone has their own editing preferences after all). Any thoughts on this @mrinalwadhwa ?

@karl-cardenas-coding
Copy link
Author

@adrianbenavides I went ahead and rebased the PR.

@mrinalwadhwa mrinalwadhwa added the HACKTOBERFEST-ACCEPTED To be used when a PR is ready to merge or after it's merged label Oct 30, 2022
@adrianbenavides
Copy link
Member

@Mergifyio rebase

@mergify
Copy link

mergify bot commented Dec 8, 2022

rebase

❌ Base branch update has failed

Git reported the following error:

Rebasing (1/9)
error: could not apply 470bf99a... docs: added docker instructions to the excersie in readme.md
Resolve all conflicts manually, mark them as resolved with
"git add/rm <conflicted_files>", then run "git rebase --continue".
You can instead skip this commit: run "git rebase --skip".
To abort and get back to the state before "git rebase", run "git rebase --abort".
Could not apply 470bf99a... docs: added docker instructions to the excersie in readme.md
Auto-merging README.md
CONFLICT (content): Merge conflict in README.md

err-code: 8DBA3

@adrianbenavides
Copy link
Member

I'm closing this PR as we have included a big change related to how we install/use the ockam binary, which makes most of the changes introduced here outdated. Thanks @karl-cardenas-coding for your work on this PR 🙏

@etorreborre etorreborre mentioned this pull request Apr 10, 2023
@mrinalwadhwa mrinalwadhwa added HACKTOBERFEST-ACCEPTED-2022 and removed HACKTOBERFEST-ACCEPTED To be used when a PR is ready to merge or after it's merged labels Sep 29, 2023
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.

None yet

3 participants