🏗 Add Lando development environment#23213
Conversation
229d97f to
c65c57f
Compare
|
@rsimha Thoughts on this? |
|
In regards to the Local AMP extension, it seems to work fine but there are few errors in the console:
I'm not sure if these are normal. |
|
/cc @ampproject/wg-infra |
rsimha
left a comment
There was a problem hiding this comment.
Hi @westonruter, sorry about the delay in reviewing this. I was traveling when I first saw it, and made a mental note to try it out locally before reviewing.
I've added a few comments and looped in a few reviewers since I don't fully understand the scope of all the changes you've made. If not all these changes are critical to your workflow, maybe you could break this up into separate PRs that can be individually tested (and reverted if necessary)?
| cmd: "yarn gulp" | ||
| watch: | ||
| service: appserver | ||
| cmd: "yarn gulp watch" |
There was a problem hiding this comment.
The sum total of all our gulp commands used during development can be found by running gulp help. Other commands that might be useful to add here: check-types, dist, unit, integration, and e2e.
|
@rsimha Thanks!
Yes, I'm happy to split it up into separate PRs. The changes to the Chrome extension are independent to the Lando environment, though the Lando environment depends on it to customize the URL. Similarly, the changes to |
|
Chrome Extension changes LGTM although I agree they should probably be a separate PR |
estherkim
left a comment
There was a problem hiding this comment.
I like the initiative to simplify the set up process for new contributors. However I have a few concerns -
- Running Lando requires a powerful machine with 16GB+ RAM and 100GB+ disk space, not one that every contributor may have. See https://docs.lando.dev/basics/installation.html#hardware-requirements
- Local dev is faster (and preferred IMO)
If it's ok, I'd like to clarify the use cases for adding Lando support before moving forward.
@estherkim Thanks for reviewing. Local dev should still be a valid path, but using Lando can be an alternative for developers who prefer a containerized environment that's also easy to use. IMO using something Docker-based should be preferred since it is more secure. |
|
(Rebased latest commits from |
I agree with @estherkim's point that due to the hardware requirements, this isn't easy for every developer to do. In general, I don't think a docker-based environment should be a part of the default recommended developer workflow for AMP, since not everyone can use it, and it's likely to increase the maintenance burden on our side. Having said that, we're definitely supportive of smaller changes to the infrastructure so that those who'd like to use environments like Lando can do so. Re: the changes in this PR, perhaps you could do the following?
@estherkim @westonruter WDYT? |
This all sounds great to me. Thanks @rsimha for providing a clear way forward. |
|
@rsimha Sounds good.
For Local AMP extension, see pull request here: #24029
See outstanding question in #23213 (comment)
👍 |
westonruter
left a comment
There was a problem hiding this comment.
I've cherry-picked the commits related to non-localhost gulp serve improvements: #24066.
…epo in non-amphtml dir" This reverts commit 57b9857.
* 'master' of github.com:ampproject/amphtml: (32 commits) ✨ Make tweet id a bindable attribute (ampproject#23953) 🏗 Update Local AMP extension to allow custom base URLs (ampproject#24029) 🏗 Improve serving from non-localhost host (ampproject#24066) Preventing half pixels. (ampproject#24050) Update callout-vendors.js (ampproject#23218) 🏗 Fixes to `check-package-manager.js` (ampproject#24060) Rename AMP_MODE to __AMP_MODE. (ampproject#24052) Story media performance metrics. (ampproject#23962) Updating Story amp-sidebar width documentation. (ampproject#23894) Fixes race condition in amp-video-iframe (ampproject#24033) Rename ampExtendedElements to __AMP_EXTENDED_ELEMENTS (ampproject#24056) 🏗🚮 Enable property inlining (ampproject#24053) ✨amp-ads: Added optional params for Directadvert network (ampproject#23724) <amp-experiment> style mutation fix and improvment (ampproject#23669) 🐛 Allow http protocol for noscript > img fallbacks for parity with amp-img (ampproject#21686) 🏗 Refactor transform-log-asserts (ampproject#24028) Automatically preconnect to source origins on page loads. (ampproject#24045) Support visibility API in the ampdoc (ampproject#23799) Amphtml visual tests should use relative path against root (ampproject#24042) FIX: check all fields' dirtiness on AMP form init (ampproject#23978) ...
|
Excellent. I merged |
There was a problem hiding this comment.
@westonruter Thanks for your patience with this PR! I've added a couple of comments that may make the config more maintainable.
About merging .lando.yml:
- I'm fine with adding it to the repo since it doesn't affect the default developer workflow, and there is precedent for adding dotfiles.
- However, I'm hesitant to actively recommend docker-based development in our dev docs because most AMP developers work on current or previous gen Macbooks that do not meet the hardware requirements. (I asked @jridgewell about this yesterday.)
In order to facilitate contributing to AMP, this PR seeks to automate much of the manual steps outlined in the Getting Started End-to-end guide and to provide a containerized development environment via Lando, which is a cross-platform user-friendly wrapper around Docker.
With Lando installed, the steps to get started with contributing to AMP would be simplified to
git cloneandlando start. Thegulp servecommand would then be run automatically so that you should be able to right away navigate tohttps://amphtml.lndo.site/as opposed tohttp://localhost:8000/.👉 Lando would be an optional path for developers to get up and running quickly for contributing in a containerized environment, as long as their machine meets the system requirements for Lando. Doing local dev directly on the host machine would continue to be an option.
Setup
Install the latest release of Lando (which includes Docker): https://github.com/lando/lando/releases (see System Requirements).
Add to your machine's
hostsfile:You'll trust the default Lando CA on your machine so that you can go to
https://amphtml.lando.sitewithout any SSL warnings.lando startOutput from running
lando startfor the first time orlando rebuildshould be:lando watchThen running
lando watchfires upgulp watchin the container:Todo