#321 #314 #302 should implement project specific config support feature #326

phuonglm · 2017-06-15T05:59:55Z

connect #321

please check with this project
https://github.com/phuonglm/iorad/tree/support-project-base-config
https://github.com/phuonglm/iorad-extension/tree/support-project-base-config

vagrant_config_override.json :

{
  "vm":{
    "synced_folders":[{ //see: http://docs.vagrantup.com/v2/synced-folders/index.html
      "_id": "0",
      "_op": "d"
    }, { // Disable /workspace sync, use project config only
      "_id": "1",
      "_op": "d"
    }]
  },
  "config_paths": [
    "workspace/iorad/iorad-dev/vagrant_config.default.json",
    "workspace/iorad-extension/iorad-dev/vagrant_config.default.json"
  ]
}

hoatle · 2017-06-18T11:20:48Z

@phuonglm please add config for https://github.com/acme101 because it's having the latest best practices.

make sure the spec specified here should be implemented #315

I'd like to see unit tests to reflect the spec and use cases.

hoatle

please see my comments to update.

hoatle · 2017-06-18T11:23:19Z

I can only review when unit tests are provided to reflect all types of use cases and specification. @phuonglm please update asap.

hoatle · 2017-06-18T11:27:37Z

I'd like to see how this works with many projects at https://github.com/teracyhq organization

CLAassistant · 2017-07-12T22:06:04Z

All committers have signed the CLA.

phuonglm · 2017-07-12T22:39:43Z

@hoale please check new update with config

{
  "vm":{
    "synced_folders":[{ //see: http://docs.vagrantup.com/v2/synced-folders/index.html
      "_id": "0",
      "_op": "d"
    }, {
      "_id": "1",
      "_op": "d"
    }
  ]
  },
  "config_paths": [
    "workspace/dev-setup/vagrant_config.default.json",
    "workspace/angular-hello-world/vagrant_config.default.json",
    "workspace/nextjs-hello-world/vagrant_config.default.json"
  ]
}

with related repository
https://github.com/phuonglm/dev-setup/tree/project-base-config
https://github.com/phuonglm/angular-hello-world/tree/project-base-config
https://github.com/phuonglm/nextjs-hello-world/tree/project-base-config
Those are just example change for example project base config. Will submit other PR after this merged.

hoatle

@phuonglm please check my comments.

hoatle · 2017-07-13T00:28:05Z

Vagrantfile

@@ -12,14 +14,36 @@ override_hash = nil
 # Check and override if exist any match JSON object from vagrant_config_override.json
 if File.exist? (File.dirname(__FILE__) + '/vagrant_config_override.json')
  override_file = File.read(File.dirname(__FILE__) + '/vagrant_config_override.json')
-
+  parsing_file = File.dirname(__FILE__) + '/vagrant_config_override.json'


not sure why we need this var here? move into the "begin" block instead? and why use that initial value? I don't see the relevance of the initial value.

hoatle · 2017-07-13T00:36:41Z

Vagrantfile

+    if data_hash.key?('config_paths')
+      data_hash['config_paths'].each do |default_config_file_path, value|
+        overide_config_file_path = default_config_file_path.gsub(/default\.json$/, "overide.json")
+        if File.exist? (default_config_file_path)


don't add a white space after ?, please fix all this style issue throughout this file.

hoatle · 2017-07-13T00:40:34Z

Vagrantfile

-        options[:bridge] = vm_network['bridge'] unless vm_network['bridge'].nil? or vm_network['bridge'].empty?
+        bridge_interface = ""
+        bridge_interface = vm_network['bridge'] unless vm_network['bridge'].nil? or vm_network['bridge'].empty?
+        bridge_interface = get_default_nic() if bridge_interface.empty?


not related to this issue, move to another branch instead.

Don't ever ever ever ever combine code from different issues into one branch, on PR, I hate that. Split please.

hoatle · 2017-07-13T00:41:03Z

lib/os.rb

@@ -0,0 +1,298 @@
+require 'rbconfig'


not related to this issue, please move out.

hoatle · 2017-07-13T00:41:46Z

lib/utility.rb

    end

    if obj1.has_key?(key)
      if value.class.name == 'Hash'
        obj1[key] = overrides(obj1[key], obj2[key])
      elsif value.class.name == 'Array'
+


why 2 new lines added here?

hoatle · 2017-07-13T00:50:39Z

spec/utility_spec.rb

@@ -287,5 +287,86 @@
        expect(new_provisioners[0]['run_list']).to eql(obj2['provisioners'][0]['run_list'])
      end
    end
+    context "given a object then override it with another object contain array" do


given an object ... with another object containing an array

hoatle · 2017-07-13T00:54:19Z

spec/utility_spec.rb

+        obj2 = {
+          "provisioners" => [{
+            "_id" => "0",
+            "_a_run_list" => []


I don't see the case why we need _a_ here? Please provide real use case (use fixture as a real data). By the way, the test failed with this:

it "all array name in new and old array must be normalize" do obj1 = {} obj2 = { "provisioners" => [{ "_id" => "0", "_a_run_list" => ["hi", "there"] }] } new_provisioners = overrides(obj1, obj2)['provisioners'] expect(new_provisioners[0]['run_list']).to eql(obj2['provisioners'][0]['_a_run_list']) end

I'd like to see the cases of:

organization A + project 1

organization A + project 2

oroganization A + project 1 + project 2

With the same data from the project configs (acme101) to see how the final config should look like.

hoatle · 2017-07-13T01:08:49Z

spec/utility_spec.rb

+        new_provisioners = overrides(obj1, obj2)['provisioners']
+        expect(new_provisioners[0]['run_list']).to eql(obj2['provisioners'][0]['run_list'])
+      end
+      it "all array name in new and old array must be normalize and value must be append" do


must be normalized and ... must be appended

hoatle · 2017-07-13T01:18:44Z

Vagrantfile

  rescue Exception => msg
    puts red(msg)
-    puts red('from vagrant_config_override.json')
-    ans = prompt yellow("some errors have occured and 'vagrant_config_override.json' file will not be used, do you want to continue? [y/N]: ")
+    puts red('from ' + parsing_file )


red('from ' + parsing_file ) => red('from ' + parsing_file)

hoatle · 2017-07-13T01:21:27Z

Vagrantfile

+    if data_hash.key?('config_paths')
+      data_hash['config_paths'].each do |default_config_file_path, value|
+        overide_config_file_path = default_config_file_path.gsub(/default\.json$/, "overide.json")
+        if File.exist? (default_config_file_path)


if the default config file does not exist => print the error message + exit immediately. This is considered a fatal error to be fixed by users immediately.

hoatle · 2017-07-13T01:29:01Z

@phuonglm btw, please sign the CLA.

hoatle · 2017-07-13T02:07:45Z

@phuonglm notice that the spec specified vagrant_config_default.json and vagrant_config_override.json convention. It's closer to vagrant_config.json and vagrant_config_override.json current convention.

vagrant_config.default.json, we should avoid using . for file names to avoid some confusion to editors (mistaken as file extensions sometimes)

hoatle · 2017-07-13T05:35:06Z

@phuonglm please implement the auto discovery path for organization config, implement this similarly to #246

By default, load workspace/dev-setup/vagrant_config_default.json if it exists (added to config_paths by default. The configuration path is: vagrant.config_paths.
Users can override this config_paths (of course), see how extension_paths work.

This is used so that users don't have to create any _override.json files if the default always works for them. Users don't have to configure anything if the default works (this works most of the time).

Just git update and the new default config will be updated, users don't have to adjust anything.

Later on, users will know what to do after these kinds of config update, see #318

hoatle · 2017-07-14T12:01:53Z

@phuonglm your commit message is messy, please update.

hoatle · 2017-07-14T12:12:37Z

Vagrantfile


+if File.exist?(File.dirname(__FILE__) + '/workspace/dev-setuo/vagrant_config_default.json')


hoatle · 2017-07-14T12:14:13Z

Vagrantfile

@@ -125,7 +183,6 @@ Vagrant.configure("2") do |config|
        end
      when 'public_network'
        options[:ip] = vm_network['ip'] unless vm_network['ip'].nil? or vm_network['ip'].strip().empty?
-        options[:bridge] = vm_network['bridge'] unless vm_network['bridge'].nil? or vm_network['bridge'].empty?


revert this

hoatle · 2017-07-14T12:15:14Z

spec/fixture/config_overide.json

+    }
+  ]
+  },
+  "config_paths": [


should not include config_paths here for best practices

btw, "config_paths" should be on the same group with "extension_paths" (under "vagrant")

hoatle · 2017-07-14T12:16:32Z

spec/fixture/org_project.json

@@ -0,0 +1,46 @@
+{


should add "config_paths" for org_project here

hoatle · 2017-07-14T12:20:06Z

spec/fixture/org_project.json

+  },
+  "provisioners": [{
+    "_id": "0",
+    "_ua_cookbooks_path": [


should use a here with "workspace/dev-setup/chef/main-cookbooks" element only, this show the use case of _a_

hoatle · 2017-07-14T12:20:36Z

spec/fixture/project1.json

@@ -0,0 +1,64 @@
+{
+  "vm": {
+    "hostname": "acme.dev",


remove hostname

hoatle · 2017-07-14T12:55:11Z

vagrant_config.json

@@ -139,9 +139,20 @@
          "action": "add" // one of add, remove. Default: add.


Make sure to add default config_paths:

{ "vagrant": { "extension_paths": [ // add paths of Vagrantfile-ext.rb files to be loaded // the path must be relative to the Vagrantfile "Vagrantfile-ext.rb" ], "config_paths": [ // add paths of config files to be loaded // the path must be relative to the Vagrantfile "workspace/dev-setup/vagrant_config_default.json" ] } }

if the default config paths are found => load them, otherwise, ignore.
If the default config paths are overridden => load them; if not found => exit with error message.
This is the same behavior with the extension_paths.

quite inconsistent for data_hash['vagrant']['config_paths'] that dev-setup entry is ignoreable while other entry is fatal error.

I see the point due to the way we introduce "_a_" and "_ua_", let's make it consistent for both config_paths and extension_paths, I'll update extensions_path behavior after when this is merged.

hoatle · 2017-07-14T16:05:59Z

spec/fixture/project1.json

+  "provisioners": [{
+    "_id": "0",
+    "_ua_cookbooks_path": [
+      "workspace/angular-hello-world/chef/main-cookbooks"


it's best practices to have "dev-setup" on the project => "workspace/angular-hello-world/dev-setup/chef/main-cookbooks"

hoatle · 2017-07-14T16:06:30Z

spec/fixture/project2.json

@@ -0,0 +1,62 @@
+{
+  "vm": {
+    "hostname": "acme.dev",


remove this

hoatle · 2017-07-14T16:06:58Z

spec/fixture/project2.json

+  "provisioners": [{
+    "_id": "0",
+    "_ua_cookbooks_path": [
+      "workspace/nextjs-hello-world/chef/main-cookbooks"


"workspace/nextjs-hello-world/dev-setup/chef/main-cookbooks"

…fic config support feature teracyhq#326

phuonglm · 2017-07-17T02:43:45Z

I updated the PR, please check.

hoatle

looks good enough to be merged, there are minor things that will be improved after this merge and some testing for real usage cases.

phuonglm assigned datphan Jun 15, 2017

phuonglm requested review from hoatle and datphan June 15, 2017 05:59

hoatle added the stag:under-review label Jun 15, 2017

hoatle assigned phuonglm and unassigned datphan Jun 18, 2017

hoatle requested changes Jun 18, 2017

View reviewed changes

phuonglm force-pushed the support-project-base-config branch from 23c46cc to 7e27157 Compare July 12, 2017 22:05

phuonglm force-pushed the support-project-base-config branch from 7e27157 to d95d38e Compare July 12, 2017 22:12

hoatle requested changes Jul 13, 2017

View reviewed changes

phuonglm force-pushed the support-project-base-config branch from d95d38e to 8576641 Compare July 14, 2017 07:42

hoatle requested changes Jul 14, 2017

View reviewed changes

phuonglm added a commit to phuonglm/dev that referenced this pull request Jul 15, 2017

teracyhq#321 teracyhq#314 teracyhq#302 should implement project speci…

c6aecec

…fic config support feature teracyhq#326

phuonglm force-pushed the support-project-base-config branch from 8576641 to c6aecec Compare July 15, 2017 18:39

teracyhq#321 teracyhq#314 teracyhq#302 should implement project speci…

47e4dfa

…fic config support feature teracyhq#326

phuonglm force-pushed the support-project-base-config branch from c6aecec to 47e4dfa Compare July 17, 2017 01:54

hoatle assigned hoatle and unassigned phuonglm Jul 17, 2017

hoatle approved these changes Jul 17, 2017

View reviewed changes

hoatle merged commit bd538d8 into teracyhq:develop Jul 17, 2017

hoatle removed the stag:under-review label Jul 17, 2017


		if File.exist?(File.dirname(__FILE__) + '/workspace/dev-setuo/vagrant_config_default.json')

		@@ -139,9 +139,20 @@
		"action": "add" // one of add, remove. Default: add.

#321 #314 #302 should implement project specific config support feature #326

#321 #314 #302 should implement project specific config support feature #326

Conversation

phuonglm commented Jun 15, 2017 • edited by hoatle Loading

hoatle commented Jun 18, 2017

hoatle left a comment

Choose a reason for hiding this comment

hoatle commented Jun 18, 2017

hoatle commented Jun 18, 2017

CLAassistant commented Jul 12, 2017 • edited Loading

phuonglm commented Jul 12, 2017

hoatle left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hoatle commented Jul 13, 2017

hoatle commented Jul 13, 2017

hoatle commented Jul 13, 2017

hoatle commented Jul 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hoatle Jul 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phuonglm commented Jul 17, 2017

hoatle left a comment

Choose a reason for hiding this comment

phuonglm commented Jun 15, 2017 •

edited by hoatle

Loading

CLAassistant commented Jul 12, 2017 •

edited

Loading

hoatle Jul 15, 2017 •

edited

Loading