-
-
Notifications
You must be signed in to change notification settings - Fork 580
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
use official d8 release for testing #503
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looked OK to me, I just had a couple of questions about (possibly?) unrelated changes. Explaining them in the PR should do the job.
if err != nil { | ||
return errors.Errorf("Failed to write site config for site %s, dir %s, err: %v", site.Name, site.Dir, err) | ||
} | ||
// Create a config object. Err is ignored as we may not have |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably just say in the PR the reason for this set up changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These changes were necessary to facilitate using a test site archive that does not contain a .ddev/config.yaml file, which is the case now that we're using the official d8 release. I also simplified a bit in a couple ways, by always setting to a test site name we define, and by removing the extra config.Read() (this is already called inside ddevapp.NewConfig()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm actually pretty happy to not assume the pre-existing .ddev. Never did like that.
pkg/testcommon/testcommon_test.go
Outdated
@@ -120,29 +120,3 @@ func TestValidTestSite(t *testing.T) { | |||
assert.Error(err, "Could not stat temporary directory after cleanup") | |||
|
|||
} | |||
|
|||
// TestInvalidTestSite ensures that errors are returned in cases where Prepare() can't download or extract an archive. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't understand the reason for removing TestInvalidTestSite
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed these test cases because they test functionality that is no longer a part of Prepare() itself, instead they are handled by GetCachedArchive. I have updated the PR with a new test which tests the same concerns TestInvalidTestSite tested for, but tests against GetCachedArchive instead.
The Problem/Issue/Bug:
OP #463 - We want to ensure ddev works with the latest drupal 8 release.
How this PR Solves The Problem:
This PR changes the archive to use the official d8 release download instead of our test/demo drupal8 repo archive. Originally, I attempted to integrate the composer-built project from drupal-composer/drupal-project, but handling through composer takes over 4m testing locally, and we don't want to slow down our tests that much. Instead, we'll use the d8 release download, and we'll just have to remember to update the URL on "major" (8.x.0) releases.
Manual Testing Instructions:
Automated Testing Overview:
Related Issue Link(s):
#463
Release/Deployment notes: