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

fix: return MTU 1440 for Gitpod, fixes #5611 #5612

Merged
merged 1 commit into from
Dec 12, 2023

Conversation

stasadev
Copy link
Member

@stasadev stasadev commented Dec 8, 2023

The Issue

This is a regression.

How This PR Solves The Issue

The project's network is created programmatically and the MTU value is not set, since it is declared only in the docker-compose.yaml file and there is no way to modify the network after it is created.

This PR returns the MTU value with a small refactoring.

Manual Testing Instructions

I don't know if this PR can be checked directly in Gitpod, so my way to check is:

  1. Checkout this PR locally
  2. Modify the source code to reverse the check for Gitpod, look at this change:
if nodeps.IsGitpod() {
    netOptions.Options = map[string]any{
        "com.docker.network.driver.mtu": "1440",
    }
}

Add ! to nodeps.IsGitpod():

if !nodeps.IsGitpod() {
    netOptions.Options = map[string]any{
        "com.docker.network.driver.mtu": "1440",
    }
}
  1. make build
  2. ddev start
  3. docker inspect ddev-project-name_default should contain this:
"Options": {
    "com.docker.network.driver.mtu": "1440"
},

Automated Testing Overview

Related Issue Link(s)

Release/Deployment Notes

Probably release a new Gitpod image after this PR goes to master (since this bug breaks Gitpod).

@stasadev stasadev requested a review from a team as a code owner December 8, 2023 19:34
@github-actions github-actions bot added the bugfix label Dec 8, 2023
Copy link

github-actions bot commented Dec 8, 2023

Comment on lines +259 to +260
labels:
com.ddev.platform: ddev
Copy link
Member Author

Choose a reason for hiding this comment

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

Why I added labels here:

This is a small addition to clean up the project network during ddev poweroff.

Some external programs (such as PhpStorm) can call docker-compose up with this template:

And in this case, when you do a ddev poweroff, the project network created by docker-compose is not removed because it has no label.

@rfay
Copy link
Member

rfay commented Dec 8, 2023

Please take a look at the history on this and check it. It seems like we had to "wire" the MTU earlier. Do you remember this @shaal ?

Anyway review of issues and commits should show it.

@rfay
Copy link
Member

rfay commented Dec 12, 2023

I thought we already fiddled with gitpod mtu.

@mk-mxp
Copy link

mk-mxp commented Dec 12, 2023

@rfay Yes, you did: #3766. But that writes it to docker-compose.yaml only. @stasadev rewrote the network launch to be handled by ddev without docker compose instead.

@shaal
Copy link
Collaborator

shaal commented Dec 12, 2023 via email

@stasadev
Copy link
Member Author

Yeah, it's a regression.
I'm going to make a new point release in the next couple of days to fix Gitpod (and introduce other stuff, like Xdebug for php8.3).

@stasadev stasadev merged commit 7a36623 into ddev:master Dec 12, 2023
16 checks passed
@stasadev stasadev deleted the 20231208_stasadev_gitpod_fix_mtu branch December 12, 2023 16:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants