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

git resource silently fails to sync or checkout when directory already exists #7347

Open
brownmike opened this issue Jun 7, 2018 · 1 comment
Labels
Focus: Resources Focus: SCM Resources Issues with the git and svn resources Type: Bug Does not work as expected.

Comments

@brownmike
Copy link

Description

The git resource's :sync action does nothing and does not fail if the target directory already exists and is not a git repo.

Chef Version

14.1.2

Platform Version

I've reproduced this in Ubuntu & macOS. After looking at the implementation, this should reproduce anywhere the git resource is usable.

Replication Case

I generated a cookbook with a default recipe that looked like this

git 'test-repo' do
  repository 'https://github.com/brownmike/test-repo.git'
  revision 'master'
  action :sync
end

I ran kitchen converge then kitchen login to verify /test-repo matched what was in my remote. I then rm -rf /test-repo/.git and added a new file to the repo on the remote. On the next kitchen converge the output implied nothing was wrong.

Client Output

       Starting Chef Client, version 14.1.12
       resolving cookbooks for run list: ["gitissues::default"]
       Synchronizing Cookbooks:
         - gitissues (0.1.0)
       Installing Cookbook Gems:
       Compiling Cookbooks...
       Converging 1 resources
       Recipe: gitissues::default
         * git[test-repo] action sync (up to date)

       Running handlers:
       Running handlers complete
       Chef Client finished, 0/1 resources updated in 01 seconds
       Downloading files from <default-ubuntu-1604>
       Finished converging <default-ubuntu-1604> (0m4.62s).

Problematic Code

From lib/chef/provider/git.rb

In the action_sync method it first checks if we're dealing with an existing git repo. If we're not, we get redirected to the action_checkout method.

def action_sync
  if existing_git_clone?
    logger.trace "#{new_resource} current revision: #{current_resource.revision} target revision: #{target_revision}"
    unless current_revision_matches_target_revision?
      fetch_updates
      enable_submodules
      logger.info "#{new_resource} updated to revision #{target_revision}"
    end
    add_remotes
  else
    action_checkout
  end
end

In the action_checkout method if the target directory is not empty, we log a line about it but otherwise continue on normally.

def action_checkout
  if target_dir_non_existent_or_empty?
    clone
    if new_resource.enable_checkout
      checkout
    end
    enable_submodules
    add_remotes
  else
    logger.trace "#{new_resource} checkout destination #{cwd} already exists or is a non-empty directory"
  end
end

Potential Solutions

  1. Explicitly fail when the target directory already exists
  2. Overwrite pre-existing target directories and update the documentation to indicate this
@lamont-granquist
Copy link
Contributor

that should fail. probably include a force or overwrite option as well.

brownmike pushed a commit to brownmike/chef that referenced this issue Jun 12, 2018
Red
===
The pre-existing assertions around this functionality can probably be deleted after these tests are made green.

Green
=====
Wrote production code to pass tests, updated mocks on related tests

Added a new requirement for the checkout action to verify the target directory doesn't exist or is empty. Then in the `action_sync` method, changed the direct call to `action_checkout` to `run_action :checkout` so it goes through all the validation steps for that action. Also needed to update a couple of test's mocks to work with this new workflow. I'll refactor a bit in the next commit.

Refactor
=======
Removing duplication, updating old code

- There was a lot of duplication in the spec that I moved into RSpec `let` values.
- Replaced duplicate call to `::File.exist?(::File.join(cwd, ".git"))` with `existing_git_clone?`
- Replaced direct call to `action_checkout` in `action_export` with the more appropriate `run_action :checkout` to go through the proper workflows.
- Removed now unreachable code branch in `action_checkout`

Signed-off-by: Mike Brown <mikbro@microsoft.com>
@tas50 tas50 added Focus: SCM Resources Issues with the git and svn resources Type: Bug Does not work as expected. labels Nov 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Focus: Resources Focus: SCM Resources Issues with the git and svn resources Type: Bug Does not work as expected.
Projects
None yet
Development

No branches or pull requests

3 participants