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: GitHub Codespaces: Adjust examples to make sure docker is running #5592

Merged
merged 8 commits into from Dec 25, 2023

Conversation

eiriksm
Copy link
Contributor

@eiriksm eiriksm commented Nov 30, 2023

The Issue

I was trying to follow the guide and something has probably changed in codespaces since then. I found postAttachCommand works though

How This PR Solves The Issue

Manual Testing Instructions

Automated Testing Overview

Related Issue Link(s)

Release/Deployment Notes

@eiriksm eiriksm requested a review from a team as a code owner November 30, 2023 18:32
@rfay rfay changed the title use postAttachCommand in examples instead docs: GitHub Codespacds: use postAttachCommand in examples instead of postCreateCommand Nov 30, 2023
@rfay rfay changed the title docs: GitHub Codespacds: use postAttachCommand in examples instead of postCreateCommand docs: GitHub Codespaces: use postAttachCommand in examples instead of postCreateCommand Nov 30, 2023
Co-authored-by: Randy Fay <randy@randyfay.com>
@rfay
Copy link
Member

rfay commented Dec 1, 2023

@mandrasch could you review this please?

Copy link
Member

@rfay rfay left a comment

Choose a reason for hiding this comment

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

Failed spellcheck due to postAttachCommand

docs/content/users/install/ddev-installation.md Outdated Show resolved Hide resolved
@mandrasch
Copy link
Collaborator

mandrasch commented Dec 1, 2023

Hi, thanks for contribution!

As posted in Discord

  • postCreateCommand is the correct event in my opinion
  • the devcontainer / codespaces team fixed it previously - docker must be available there, otherwise it's a bug in docker-in-docker on their side (universal image)

So please don't remove this from the docs, it's the correct event to install things on first creation of a codespace (e.g. install scripts, etc.)

Saw this in other codespaces demos as well, they all used postCreate

(Scripts / routines would run twice if postAttach is used and the container is opened again as far as I understand - postCreate is really for the first initial steps on a fresh codespace)

Depends on the use case of course - we could add postAttach to the docs as second option?

(Haven't worked with postAttach yet)

Please change PR accordingly if relevant / useful in your opinion :)

Cheers

@rfay rfay marked this pull request as draft December 1, 2023 01:13
@rfay
Copy link
Member

rfay commented Dec 1, 2023

I'm going to change this to draft status for now; @mandrasch if you can keep us up-to-date with the upstream problem it will be appreciated.

@eiriksm
Copy link
Contributor Author

eiriksm commented Dec 1, 2023

Amazing, thanks! 😍

I'm just glad it's nothing wrong with the docs or ddev, so I just agree with everything said in this thread basically

@mandrasch
Copy link
Collaborator

Amazing, thanks! 😍

I'm just glad it's nothing wrong with the docs or ddev, so I just agree with everything said in this thread basically

👍 👍

just fiy: I submitted a new issue here devcontainers/features#780

@mandrasch
Copy link
Collaborator

fyi: new comment devcontainers/features#780 (comment)

@rfay
Copy link
Member

rfay commented Dec 15, 2023

It does sound like perhaps a new approach is in order @mandrasch ? based on

@mandrasch
Copy link
Collaborator

It does sound like perhaps a new approach is in order @mandrasch ? based on

yeah... I'll see when I'll find time in the next weeks and I'll post here again. happy travelling in the meantime!

(if anyone else here reading this and wants to try it sooner - please feel free to test the new retry approach 😎)

@eiriksm
Copy link
Contributor Author

eiriksm commented Dec 21, 2023

Tested the script and slightly altered it. Seems to work ✌️

@eiriksm
Copy link
Contributor Author

eiriksm commented Dec 21, 2023

Updated with new command suggestion. Removed the inline bash example since that must be considered unstable at this point I guess 🤓✌️

@eiriksm eiriksm marked this pull request as ready for review December 21, 2023 16:35
@mandrasch
Copy link
Collaborator

mandrasch commented Dec 22, 2023

Awesome work @eiriksm, thanks very much for taking the time to solve this and editing the docs! 👏 🎉 🎉 🎉

(I'll update my demo repo https://github.com/mandrasch/ddev-craftcms-vite in the next days or weeks with your new retry logic 🥳 )

@mandrasch
Copy link
Collaborator

Works fine for me, good to go from my point of view! 👍

Thanks very much again!

One last thing: @eiriksm, could you please change the title of your PR?

@eiriksm eiriksm changed the title docs: GitHub Codespaces: use postAttachCommand in examples instead of postCreateCommand docs: GitHub Codespaces: Adjust examples to make sure docker is running Dec 23, 2023
@eiriksm
Copy link
Contributor Author

eiriksm commented Dec 23, 2023

👍done

Thanks for your hard work @mandrasch ♥️

@stasadev stasadev merged commit 8a7b610 into ddev:master Dec 25, 2023
12 checks passed
@stasadev
Copy link
Member

Thank you!

@eiriksm eiriksm deleted the patch-1 branch December 25, 2023 19:17
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

4 participants