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

Add config directory #330

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

scottalan
Copy link
Member

No description provided.

grunt.config(['symlink', 'config'], {
src: '<%= config.srcPaths.drupal %>/config',
dest: drupal.configPath()
});
Copy link
Contributor

Choose a reason for hiding this comment

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

This code works just fine, but it does read as out of order to add the task before defining it.

Copy link
Member Author

Choose a reason for hiding this comment

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

test/build.js Outdated
var src = fs.existsSync('src/config');
if (src) {
fs.existsSync('build/html/config', function(exists) {
assert.ok(exists);
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe I'm fuzzy on this, but if we have a test where the src/config doesn't exist, we should confirm the build/html/config doesn't exist. If we don't, why include the check?

Copy link
Member Author

Choose a reason for hiding this comment

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

@grayside
Copy link
Contributor

Does this deprecate #323?

@scottalan
Copy link
Member Author

@grayside yes, this deprecates #323

I forked the repo, vs pushing straight to this branch in origin.

test/build.js Outdated
done();
} else if (html && !src) {
assert.ok(false, 'Error: build/html/config exists but src/config does not');
done();
Copy link
Contributor

Choose a reason for hiding this comment

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

I appreciate the thoroughness here. Do we have Drupal 7 test cases executing that won't have the config directory? If so, that seems like it deserves a separate test case.

@grayside
Copy link
Contributor

Re-triggering travis tests.

@grayside grayside closed this Oct 31, 2017
@grayside grayside reopened this Oct 31, 2017
…e exists at 'src/config'

 * Adding documentation to  10_BUILD.md in reference to the config directory.
 * Updating the test (build.js) to use fs.existsSync for my test
 * Reorder task/definition, Rewrite the test for the config directories
 * Add a test case for both Drupal 8 & 7
it('config directory should exist if provided ', function(done) {
var src = fs.existsSync('src/config');
var html = fs.existsSync('build/html/config');
if (drupalCore === '8') {
Copy link
Member Author

Choose a reason for hiding this comment

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

I believe strict equality is fine here based on what I'm seeing here:

Setting environment variables from .travis.yml
$ export NVM_NODE_VERSION="4"
$ export GDT_DRUPAL_CORE="7"

@scottalan
Copy link
Member Author

scottalan commented Nov 6, 2017

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.

3 participants