Skip to content

Conversation

@bzp2010
Copy link
Contributor

@bzp2010 bzp2010 commented Nov 21, 2024

Description

Add devcontainer support to the project, which is expected to cover Linux, Windows (WSL2), macOS OSs, and the amd64, arm64 CPU ISAs, i.e. the environments used by most developers.

Basically, the container provides an isolated environment from the host, the runtime and toolset required for APISIX development are built in the container build, they will be available out of the box and developers no longer need to build and install them on the host. Just start dev container, wait a few moments, and it's ready to go.

Significantly simplify the parts that may be unfamiliar to developers and just focus on writing Lua code and test cases.

In addition to this, etcd is ready, maintained by docker-compose, and accessible on the local loopback.

Checklist

  • I have explained the need for this PR and the problem it solves
  • I have explained the changes or the new features added to this PR
  • I have added tests corresponding to this change
  • I have updated the documentation to reflect this change
  • I have verified that this change is backward compatible (If not, please discuss on the APISIX mailing list first)

@bzp2010
Copy link
Contributor Author

bzp2010 commented Nov 21, 2024

I have tested this on Windows WSL2 + Docker Desktop and macOS + OrbStack, where macOS is an arm64 based m4 chip.
Developers don't need to make any individual adjustments to any of the above environments, they will automatically do the right thing. (e.g. Installs the arm64-based binaries)

However, I don't have a desktop system running Linux, which would require someone else to help test, but that should not be a problem.

@bzp2010
Copy link
Contributor Author

bzp2010 commented Nov 21, 2024

The error in the CI is not related to this PR, it also fails on other PRs, will investigate it later.

UPDATE

I have fixed the issue in #11769.

@bzp2010 bzp2010 force-pushed the bzp/feat-devcontainer branch from 5533908 to 44bc599 Compare November 21, 2024 12:43
@bzp2010 bzp2010 marked this pull request as ready for review November 22, 2024 04:48
@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. enhancement New feature or request labels Nov 22, 2024
@bzp2010 bzp2010 requested a review from SkyeYoung November 22, 2024 06:19
@SkyeYoung
Copy link
Member

🆒 I will help with testing using Manjaro KDE Plasma(amd64) + Docker Engine.

@SkyeYoung
Copy link
Member

SkyeYoung commented Nov 22, 2024

Tested in Manjaro KDE Plasma (amd64) + Docker Engine:

image image

But I encountered some problems during the test:

image image

Error Log

error.log

@github-actions
Copy link

github-actions bot commented Feb 6, 2025

This pull request has been marked as stale due to 60 days of inactivity. It will be closed in 4 weeks if no further activity occurs. If you think that's incorrect or this pull request should instead be reviewed, please simply write any comment. Even if closed, you can still revive the PR at any time or discuss it on the [email protected] list. Thank you for your contributions.

@github-actions github-actions bot added the stale label Feb 6, 2025
@bzp2010 bzp2010 removed the stale label Mar 16, 2025
@bzp2010 bzp2010 requested a review from SkyeYoung March 17, 2025 04:40
@bzp2010
Copy link
Contributor Author

bzp2010 commented Mar 20, 2025

This PR has been renovated, where the tests can run correctly, except for a problem that needs to be solved by #12070.

@SkyeYoung
Copy link
Member

This PR has been renovated, where the tests can run correctly, except for a problem that needs to be solved by #12070.

Looks like #12070 has been merged.

@dosubot dosubot bot added size:XL This PR changes 500-999 lines, ignoring generated files. and removed size:L This PR changes 100-499 lines, ignoring generated files. labels Mar 30, 2025
@bzp2010
Copy link
Contributor Author

bzp2010 commented Mar 30, 2025

Merging the master branch to fix #12070 doesn't seem to be successful, I'll rebase and force push.

image

@bzp2010 bzp2010 force-pushed the bzp/feat-devcontainer branch from cf3b2f4 to 183ad13 Compare March 30, 2025 14:40
@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. and removed size:XL This PR changes 500-999 lines, ignoring generated files. labels Mar 30, 2025
@SkyeYoung
Copy link
Member

SkyeYoung commented Mar 31, 2025

Still got some error logs: It seems that there will be many problems caused by personal environmental factors

CleanShot 2025-03-31 at 21 56 12@2x

🤔

Copy link
Contributor

@Baoyuantop Baoyuantop left a comment

Choose a reason for hiding this comment

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

Works well on my arm macOS.


:::note

The following uses Visual Studio Code, which has built-in integration with Dev Containers.
Copy link
Contributor

Choose a reason for hiding this comment

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

Need to install the Dev Containers extension?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@bzp2010 bzp2010 May 17, 2025

Choose a reason for hiding this comment

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

Usually, it is pre-included. I mentioned that "which has built-in integration"

@bzp2010
Copy link
Contributor Author

bzp2010 commented May 17, 2025

I confirmed that the test failure is not related to this PR, so maybe we can merge this PR first and continue to improve usability in subsequent PRs. This PR has been open for too long.

@bzp2010 bzp2010 requested review from Baoyuantop and SkyeYoung May 17, 2025 16:40
Copy link
Member

@juzhiyuan juzhiyuan 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 tried locally and works well

Copy link
Member

@SkyeYoung SkyeYoung left a comment

Choose a reason for hiding this comment

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

I tried it again locally, and it is still the previous environment.

make run and FLUSH_ETCD=1 prove -Itest-nginx/lib -I. -r t/admin/api.t works.

But make test still have some problems.

image

@bzp2010
Copy link
Contributor Author

bzp2010 commented Jun 1, 2025

@juzhiyuan @SkyeYoung
image

These warnings will be fixed in a later PR. I will merge this PR first.

@bzp2010 bzp2010 merged commit f1630d2 into apache:master Jun 1, 2025
39 checks passed
@github-project-automation github-project-automation bot moved this from 👀 In review to ✅ Done in ⚡️ Apache APISIX Roadmap Jun 1, 2025
Crazy-xyr pushed a commit to Crazy-xyr/apisix that referenced this pull request Jun 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request size:L This PR changes 100-499 lines, ignoring generated files.

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

4 participants