Merge branch 'master' into kpaulisse-git-submodule-example

Author: kpaulisse

.rubocop.yml

@@ -22,6 +22,9 @@ Metrics/AbcSize:
   Max: 100
   Severity: warning
 
+Metrics/BlockLength:
+  Enabled: false
+
 Metrics/ClassLength:
   Max: 500
   Severity: warning
@@ -40,12 +43,33 @@ Metrics/PerceivedComplexity:
 
 # Indentation
 CaseIndentation:
-  IndentWhenRelativeTo: end
   IndentOneStep: false
 
 Lint/EndAlignment:
-  AlignWith: variable
+  EnforcedStyleAlignWith: variable
 
 # To let us use a '-' in the gem name
 Style/FileName:
   Enabled: false
+
+# To fix later
+Style/PercentLiteralDelimiters:
+  Enabled: false
+Style/VariableNumber:
+  Enabled: false
+Security/YAMLLoad:
+  Enabled: false
+Lint/AmbiguousBlockAssociation:
+  Enabled: false
+Lint/UnifiedInteger:
+  Enabled: false
+Style/GuardClause:
+  Enabled: false
+Style/EmptyMethod:
+  Enabled: false
+Style/SymbolArray:
+  Enabled: false
+Style/IndentHeredoc:
+  Enabled: false
+Style/ExtraSpacing:
+  Enabled: false

doc/advanced-dynamic-ignores.md

@@ -2,6 +2,8 @@
 
 Using the `--ignore-tags` command line option, it is possible to ignore all resources with particular Puppet tags. This allows dynamic ignoring of wrappers or other resources that are not of interest.
 
+NOTE: This option is separate and distinct from `--include-tags`, which controls whether differences in tags themselves will appear as a difference. For more on `--include-tags`, consult the [options reference](/doc/optionsref.md).
+
 ## Getting Started
 
 To use ignored tags, you first need to decide what the name of your tag will be. The standard is `ignored_octocatalog_diff`.

doc/dev/releasing.md

@@ -4,9 +4,11 @@ The project maintainers are responsible for bumping the version number, regenera
 
 ## Local testing
 
-To test the new version of `octocatalog-diff` in the Puppet repository:
+*This procedure is performed by a GitHubber.*
 
-0. In the Puppet checkout, start a new branch based off master.
+To test the new version of `octocatalog-diff` in the GitHub Puppet repository:
+
+0. In a checkout of the GitHub Puppet repository, start a new branch based off master.
 0. In the `octocatalog-diff` checkout:
   - Ensure that the desired branch is checked out.
   - Choose a unique internal version number which has never been used in CI. A good guideline is that if you're planning to release a version `0.6.0` then for these tests, use `0.6.0a`, `0.6.0b`, ...
@@ -23,25 +25,62 @@ To test the new version of `octocatalog-diff` in the Puppet repository:
 
 0. Back in the Puppet checkout, ensure that the changes are as expected (updates to Gemfile / Gemfile.lock, addition of new gem). Push the change and build appropriate CI job(s) to validate the changes.
 
-## Merging
+## Merging one PR
+
+This section is useful when releasing a new version based on one PR submitted by a contributor. Following this workflow is important so that the contributor gets appropriate credit in the revision history for his or her work.
+
+0. In your local checkout, start a new branch based off master.
+0. Add the contributor's repository as a remote. For example:
+
+  ```
+  git remote add octocat https://github.com/octocat/octocatalog-diff.git
+  ```
+
+0. Merge in the contributor's branch into your own. For example:
+
+  ```
+  git merge octocat/some-branch
+  ```
+
+0. Update `.version` and `doc/CHANGELOG.md` appropriately. In CHANGELOG you should link to the PR submitted by the contributor (not the PR you're creating now).
+0. Commit your changes to `.version` and `doc/CHANGELOG.md`.
+0. If necessary, auto-generate the build documentation, and commit the changes to your branch.
+
+  ```
+  rake doc:build
+  ```
+
+0. Open a Pull Request based on your branch. Confirm that the history is correct, showing the contributor's commits first, and then your commit(s) updating the version file, change log, and/or auto-generated documentation.
+0. Ensure that CI tests are all passing.
+0. Ensure that you've performed "local testing" within GitHub (typically, ~1 day) to confirm the changes.
+0. Merge your PR and delete your branch.
+0. Confirm that the contributor's PR now appears as merged, and any associated issues have been closed.
+
+## Merging multiple PRs
+
+If multiple PRs will constitute a release, it's generally easier to merge each such PR individually, and then create a separate PR afterwards to update the necessary files.
 
-0. If necessary, complete a Pull Request to update the [version file](/.version).
-0. If necessary, auto-generate the build documentation.
+0. Merge all constituent PRs and ensure that any associated issues have been closed.
+0. Create your own branch based off master.
+0. Update `.version` and `doc/CHANGELOG.md` appropriately. In CHANGELOG you should link to the PR submitted by the contributor (not the PR you're creating now).
+0. Commit your changes to `.version` and `doc/CHANGELOG.md`.
+0. If necessary, auto-generate the build documentation, and commit the changes to your branch.
 
   ```
   rake doc:build
   ```
 
+0. Open a Pull Request based on your branch.
 0. Ensure that CI tests are all passing.
-0. Merge and delete the branch.
+0. Ensure that you've performed "local testing" within GitHub (typically, ~1 day) to confirm the changes.
+0. Merge your PR and delete your branch.
 
 ## Releasing
 
 Generally, a new release will correspond to a merge to master of one or more Pull Requests.
 
 0. Ensure that all changes associated with the release have been merged to master.
-  - Merge all Pull Requests associated with release.
-  - If necessary, complete a Pull Request to update the [change log](/doc/CHANGELOG.md).
+  - Merge all Pull Requests associated with release, including the version number bump, change log update, etc.
   - If necessary (for significant changes), complete a Pull Request to update the top-level README file.
 0. Ensure the the master branch is checked out on your system.
 0. Run the release procedure:

lib/octocatalog-diff/catalog-diff/differ.rb

@@ -266,11 +266,8 @@ def filter_and_cleanup(catalog_resources)
               hsh[k] = cleansed_param unless cleansed_param.nil? || cleansed_param.empty?
             elsif k == 'tags'
               # The order of tags is unimportant. Sort this array to avoid false diffs if order changes.
-              # Also if tags is empty, don't add. Most uses of catalog diff will want to ignore tags,
-              # and if you're ignoring tags you won't get here anyway. Also, don't add empty array of tags.
-              unless @opts[:ignore_tags]
-                hsh[k] = v.sort if v.is_a?(Array) && v.any?
-              end
+              # Also if tags is empty, don't add.
+              hsh[k] = v.sort if v.is_a?(Array) && v.any?
             elsif k == 'file' || k == 'line'
               # We don't care, for the purposes of catalog-diff, from which manifest and line this resource originated.
               # However, we may report this to the user, so we will keep it in here for now.

lib/octocatalog-diff/catalog-util/builddir.rb

@@ -234,7 +234,7 @@ def install_hiera_config(logger, options)
 
         # Munge datadir in hiera config file
         obj = YAML.load_file(file_src)
-        (obj[:backends] || %w(yaml json)).each do |key|
+        ([obj[:backends]].flatten || %w(yaml json)).each do |key|
           next unless obj.key?(key.to_sym)
           if options[:hiera_path_strip].is_a?(String)
             next if obj[key.to_sym][:datadir].nil?

lib/octocatalog-diff/catalog.rb

@@ -201,29 +201,56 @@ def validate_references
       end
       return if missing.empty?
 
-      # At this point there is at least one broken/missing reference. Format an error message and
-      # raise. Error message will look like this:
-      # ---
-      # Catalog has broken references: exec[subscribe caller 1] -> subscribe[Exec[subscribe target]];
-      # exec[subscribe caller 2] -> subscribe[Exec[subscribe target]]; exec[subscribe caller 2] ->
-      # subscribe[Exec[subscribe target 2]]
-      # ---
+      # At this point there is at least one broken/missing reference. Format an error message and raise.
+      errors = format_missing_references(missing)
+      plural = errors =~ /;/ ? 's' : ''
+      raise OctocatalogDiff::Errors::ReferenceValidationError, "Catalog has broken reference#{plural}: #{errors}"
+    end
+
+    private
+
+    # Private method: Format the name of the source file and line number, based on compilation directory and
+    # other settings. This is used by format_missing_references.
+    # @param source_file [String] Raw source file name from catalog
+    # @param line_number [Fixnum] Line number from catalog
+    # @return [String] Formatted source file
+    def format_source_file_line(source_file, line_number)
+      return '' if source_file.nil? || source_file.empty?
+      filename = if compilation_dir && source_file.start_with?(compilation_dir)
+        stripped_file = source_file[compilation_dir.length..-1]
+        stripped_file.start_with?('/') ? stripped_file[1..-1] : stripped_file
+      else
+        source_file
+      end
+      "(#{filename.sub(%r{^environments/production/}, '')}:#{line_number})"
+    end
+
+    # Private method: Format the missing references into human-readable text
+    # Error message will look like this:
+    # ---
+    # Catalog has broken references: exec[subscribe caller 1](file:line) -> subscribe[Exec[subscribe target]];
+    # exec[subscribe caller 2](file:line) -> subscribe[Exec[subscribe target]]; exec[subscribe caller 2](file:line) ->
+    # subscribe[Exec[subscribe target 2]]
+    # ---
+    # @param missing [Array] Array of missing references
+    # @return [String] Formatted references
+    def format_missing_references(missing)
+      unless missing.is_a?(Array) && missing.any?
+        raise ArgumentError, 'format_missing_references() requires a non-empty array as input'
+      end
+
       formatted_references = missing.map do |obj|
         # obj[:target_value] can be a string or an array. If it's an array, create a
         # separate error message per element of that array. This allows the total number
         # of errors to be correct.
-        src = "#{obj[:source]['type'].downcase}[#{obj[:source]['title']}]"
+        src_ref = "#{obj[:source]['type'].downcase}[#{obj[:source]['title']}]"
+        src_file = format_source_file_line(obj[:source]['file'], obj[:source]['line'])
         target_val = obj[:target_value].is_a?(Array) ? obj[:target_value] : [obj[:target_value]]
-        target_val.map { |tv| "#{src} -> #{obj[:target_type].downcase}[#{tv}]" }
-      end
-      formatted_references.flatten!
-      plural = formatted_references.size == 1 ? '' : 's'
-      errors = formatted_references.join('; ')
-      raise OctocatalogDiff::Errors::ReferenceValidationError, "Catalog has broken reference#{plural}: #{errors}"
+        target_val.map { |tv| "#{src_ref}#{src_file} -> #{obj[:target_type].downcase}[#{tv}]" }
+      end.flatten
+      formatted_references.join('; ')
     end
 
-    private
-
     # Private method: Given a list of resources to check, return the references from
     # that list that are missing from the catalog. (An empty array returned would indicate
     # all references are present in the catalog.)

octocatalog-diff.gemspec

@@ -1,6 +1,6 @@
 require_relative 'lib/octocatalog-diff/version'
 
-DEFAULT_PUPPET_VERSION = '4.5.3'.freeze
+DEFAULT_PUPPET_VERSION = '4.10.0'.freeze
 
 Gem::Specification.new do |s|
   s.required_ruby_version = '>= 2.0.0'
@@ -26,19 +26,19 @@ EOF
   s.add_runtime_dependency 'diffy', '>= 3.1.0'
   s.add_runtime_dependency 'httparty', '>= 0.11.0'
   s.add_runtime_dependency 'hashdiff', '>= 0.3.0'
-  s.add_runtime_dependency 'parallel', '>= 1.9.0'
+  s.add_runtime_dependency 'parallel', '>= 1.11.1'
   s.add_runtime_dependency 'rugged', '>= 0.25.0b2'
 
   s.add_development_dependency 'rspec', '~> 3.4.0'
   s.add_development_dependency 'rake', '11.2.2'
   s.add_development_dependency 'parallel_tests', '2.7.1'
   s.add_development_dependency 'rspec-retry', '0.5.0'
 
-  s.add_development_dependency 'rubocop', '~> 0.35'
+  s.add_development_dependency 'rubocop', '= 0.48.1'
 
   s.add_development_dependency 'puppetdb-terminus', '3.2.4'
 
-  s.add_development_dependency 'simplecov'
+  s.add_development_dependency 'simplecov', '>= 0.14.1'
   s.add_development_dependency 'simplecov-json'
 
   if ENV['PUPPET_VERSION']

script/cibuild

@@ -6,22 +6,16 @@
 # with one or more Puppet versions, with PUPPET_VERSIONS set to a space-separated list
 # of versions to test.
 
-[ -z "$PUPPET_VERSIONS" ] && export PUPPET_VERSIONS='3.8.7 4.5.3'
+[ -z "$PUPPET_VERSIONS" ] && export PUPPET_VERSIONS='3.8.7 4.5.3 4.10.0'
 [ -z "$RUBOCOP_TEST" ] && export RUBOCOP_TEST='true'
 [ -z "$RSPEC_TEST" ] && export RSPEC_TEST='true'
 
-# FIXME: Needed only for Janky. Remove for travis.
-if env | grep ^JANKY_ >/dev/null 2>&1; then
-  export PATH=/usr/share/rbenv/shims:$PATH
-  export RBENV_VERSION=2.1.2-github
-fi
-
 echo 'Starting script/cibuild'
 
 DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" && cd .. && pwd )"
 
 # Create a temporary file to capture output of various steps.
-export OUTPUT_FILE=$(mktemp)
+export OUTPUT_FILE="$(mktemp)"
 function cleanup() {
   rm -rf "$OUTPUT_FILE"
 }
@@ -79,12 +73,12 @@ if [ "$RSPEC_TEST" = "true" ]; then
 
     # Make sure `script/puppet` returns the desired puppet version
     pv_test=$( "${DIR}/script/puppet" --version 2>&1 )
-    if [ $? -eq 0 -a "$pv_test" = "$pv" ]; then
+    if [ $? -eq 0 ] && [ "$pv_test" = "$pv" ]; then
       echo "Confirmed Puppet version = ${pv_test}"
     else
       echo "Failures:"
       echo "- Unable to confirm that Puppet version = ${pv}"
-      echo $pv_test
+      echo "$pv_test"
       RSPEC_EXITCODE=255
     fi
 
@@ -108,4 +102,4 @@ fi
 echo "Finished script/cibuild:"
 [ "$RUBOCOP_EXITCODE" -ge 0 ] && echo "  - rubocop: exit ${RUBOCOP_EXITCODE}"
 [ "$RSPEC_EXITCODE" -ge 0 ] && echo "  - rspec:   exit ${RSPEC_EXITCODE}"
-if [ "$RUBOCOP_EXITCODE" -gt 0 -o "$RSPEC_EXITCODE" -gt 0 ]; then exit 1; else exit 0; fi
+if [ "$RUBOCOP_EXITCODE" -gt 0 ] || [ "$RSPEC_EXITCODE" -gt 0 ]; then exit 1; else exit 0; fi

spec/octocatalog-diff/fixtures/catalogs/include-tags-new.json

@@ -0,0 +1,41 @@
+{
+  "document_type": "Catalog",
+  "data": {
+    "tags": ["settings"],
+    "name": "my.rspec.node",
+    "version": "production",
+    "environment": "production",
+    "resources": [
+      {
+        "type": "Stage",
+        "title": "main",
+        "tags": ["stage"],
+        "exported": false,
+        "parameters": {
+          "name": "main"
+        }
+      },
+      {
+        "type": "Class",
+        "title": "Settings",
+        "tags": ["class","settings"],
+        "exported": false
+      },
+      {
+        "type": "File",
+        "title": "/tmp/foo",
+        "tags": ["tag-one-new","tag-too","ignore-tag__file"],
+        "parameters": {
+          "content": "foofoo",
+          "owner": "root"
+        }
+      }
+    ],
+    "classes": [
+      "settings"
+    ]
+  },
+  "metadata": {
+    "api_version": 1
+  }
+}

spec/octocatalog-diff/fixtures/catalogs/include-tags-old.json

@@ -0,0 +1,41 @@
+{
+  "document_type": "Catalog",
+  "data": {
+    "tags": ["settings"],
+    "name": "my.rspec.node",
+    "version": "production",
+    "environment": "production",
+    "resources": [
+      {
+        "type": "Stage",
+        "title": "main",
+        "tags": ["stage"],
+        "exported": false,
+        "parameters": {
+          "name": "main"
+        }
+      },
+      {
+        "type": "Class",
+        "title": "Settings",
+        "tags": ["class","settings"],
+        "exported": false
+      },
+      {
+        "type": "File",
+        "title": "/tmp/foo",
+        "tags": ["tag-one","tag-two"],
+        "parameters": {
+          "content": "foofoo",
+          "owner": "root"
+        }
+      }
+    ],
+    "classes": [
+      "settings"
+    ]
+  },
+  "metadata": {
+    "api_version": 1
+  }
+}