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

Support Codespaces #341

Merged
merged 24 commits into from
Jul 18, 2022
Merged

Support Codespaces #341

merged 24 commits into from
Jul 18, 2022

Conversation

rmccue
Copy link
Member

@rmccue rmccue commented Sep 19, 2021

Early draft of just getting things working. Built on #40.

One of the bigger changes here is changing how Minio works. Rather than using the automatic bucket domain building, this changes it to be more in line with the other tools with a static domain. Additionally, it disables the TLS validation, since Minio has the wrong certificate (slash, self-signed? dunno) for these purposes. (nb, I haven’t updated this for the latest changes which added the Traefik routing; that work probably wants to go back into #40 too)

With these changes, it’s zero-configuration to work on Codespaces. Just start a Codespace with an Altis project, and composer install && composer server start, and the linked domain will just work. That said, you can also use a simplified dev container for a bit speedier setup.

This also works if you use Codespaces with VS Code desktop, but this instead uses localhost as the host, which is a bit annoying. Needs to be disabled somehow? I think there’s a VS Code setting for it idk

Needs more testing, and I am probably bad at code, so there might be cleaner ways to do some of the stuff. Also needs docs (and maybe we get a blog post up for it?)

(I’ve set the humanmade org to have $100 worth of credits, since we’re now out of the free period for Codespaces.)

joehoyle and others added 10 commits November 29, 2020 19:11
With the "HTTPS required" natire of `.dev` domains, the fact that we
currently have to issue a fully signed wildcard cert (`.dev` doesn't
support self signed certs), and then the fact that we can't support
subdomain installs in local-server; I think the days of `altis.dev` are
probably numbered. This PR atleast makes this TLD fully configurable, to
something more `.local` or `.altis.local`.

In doing so, I also added an option to set `secure` to `true` / `false`,
so HTTP-only can then be supported with TLDs other than `.dev`.

At this point I don't think we need to publicly document this
neccesarily, but I think we are going to need to move away from this
hardcoding of altis.dev. If nothing else, this makes the code more
configurable in special use cases.
Co-authored-by: Robert O'Rourke <[email protected]>
Rather than letting AWS generate the domain, override to set explicit to
the Minio domain. In the process, also changes the Minio domain to be
fixed.
@rmccue rmccue mentioned this pull request Sep 22, 2021
@roborourke
Copy link
Contributor

Not super into the secure switch here but I guess browser APIs that require https should work on localhost. If that's needed for zero config set up then all good.

@rmccue
Copy link
Member Author

rmccue commented Sep 22, 2021

Oh, the secure bit is from @joehoyle's original PR I think?

@joehoyle
Copy link
Member

Not super into the secure switch here but I guess browser APIs that require https should work on localhost. If that's needed for zero config set up then all good.

Yeah that was from my PR, it was basically a way to support other domains etc until we get a way to generate the SSL certs etc.

@roborourke
Copy link
Contributor

It feels like a good candidate for packaging up the functionality, e.g. in a WP plugin using set_url_scheme() would have the desired effect. Fewer ternaries cluttering up the place.

@rmccue can you test this with the recent changes to the Minio frontends?

@joehoyle
Copy link
Member

@roborourke im not sure I follow-- how would a wp plugin change all the local server configuration to run services as http rather than https?

@roborourke
Copy link
Contributor

Sorry I just meant the set_url_scheme() function provided by WordPress could be useful here.

@joehoyle
Copy link
Member

@rmccue anything a blocker here now? is there hope to get this in v9?

@roborourke
Copy link
Contributor

Linter checks are failing still, and I think it needs to rebased on master or have master merged into it.

Copy link
Contributor

@roborourke roborourke left a comment

Choose a reason for hiding this comment

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

Couple of tweaks to do for now, will need some documentation once the functionality is all confirmed.

I believe we need Tachyon to optionally support non-verified S3 requests as well as that's currently broken in Codespaces.

inc/composer/class-docker-compose-generator.php Outdated Show resolved Hide resolved
inc/composer/class-command.php Outdated Show resolved Hide resolved
@@ -15,6 +15,7 @@
's3' => true,
'tachyon' => true,
'analytics' => true,
'codespaces_integration' => true,
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't really understand the purpose of this config option.

Copy link
Member Author

Choose a reason for hiding this comment

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

We detect the CODESPACES environment variable and override a bunch of stuff if it's set. If you didn't want those overrides applied, this allows this to turn those off.

Copy link
Contributor

Choose a reason for hiding this comment

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

I get that much but why wouldn't you want those overrides?

Copy link
Member Author

Choose a reason for hiding this comment

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

If you wanted to set up different domains for testing purposes; without this, there's no way to avoid codespaces overriding the settings, so you wouldn't be able to test other domains/etc.

@shadyvb
Copy link
Contributor

shadyvb commented Apr 23, 2022

Refreshed rebased and extended at #466 , addressed feedback, closing this one down, but keeping the branch for comparison and referencing.

@shadyvb shadyvb closed this Apr 23, 2022
shadyvb added a commit that referenced this pull request Apr 27, 2022
@shadyvb
Copy link
Contributor

shadyvb commented May 3, 2022

Reopening after separating concerns between SSL and Codespaces support.

@shadyvb shadyvb reopened this May 3, 2022
@shadyvb shadyvb marked this pull request as ready for review May 17, 2022 14:26
@shadyvb shadyvb requested a review from roborourke May 17, 2022 14:29
@shadyvb
Copy link
Contributor

shadyvb commented May 17, 2022

Couple of remaining dependencies:

  • Need to add the dev container so we can set PHP version to 8
  • Need to fix the PHP 8 dependency issues in dev-tools/wp-browser

Copy link
Contributor

@roborourke roborourke left a comment

Choose a reason for hiding this comment

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

Not tested myself yet but looks good - just one comment stating a todo to check on, guessing it just needs removing.

Good stuff!

inc/composer/class-docker-compose-generator.php Outdated Show resolved Hide resolved
@shadyvb shadyvb requested a review from roborourke July 18, 2022 09:06
@roborourke roborourke merged commit d86fd45 into master Jul 18, 2022
@roborourke roborourke deleted the support-codespaces branch July 18, 2022 10:32
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