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.latest with force_clone fails when it can't create a target directory that already exists #40524

Merged
merged 2 commits into from
Apr 6, 2017

Conversation

zer0def
Copy link
Contributor

@zer0def zer0def commented Apr 4, 2017

What does this PR do?

Fixes the force_clone option in git.latest state, by removing the target directory's contents, instead of target directory. Previous behavior runs into a corner case where target directory can't be recreated due to user's lacking priviledges to write in target's parent dir.

What issues does this PR fix or reference?

#31363

Reference SLS

{% set target_dir = '/tmp/asdf/asdf' %}
{% set username = 'asdf' %}

step1:
  pkg.installed:
    - name: git
    - refresh: true
    - reload_modules: true
  user.present:
    - name: {{ username }}
  file.directory:
    - name: {{ target_dir.split('/')[:-1]|join('/') }}
    - user: root

step2:
  file.directory:
    - name: {{ target_dir }}
    - user: {{ username }}
    - require:
      - user: step1
      - file: step1

step3:
  file.managed:
    - name: {{ target_dir }}/asdf
    - user: root
    - require:
      - file: step2
  git.latest:
    - name: https://github.com/saltstack/salt
    - target: {{ target_dir }}
    - user: {{ username }}
    - force_clone: true
    - require:
      - file: step3
      - pkg: step1

Previous Behavior

ubuntu1:
  Name: git - Function: pkg.installed - Result: Clean Started: - 14:31:53.923285 Duration: 57.604 ms
  Name: asdf - Function: user.present - Result: Clean Started: - 14:31:53.987073 Duration: 8.299 ms
  Name: /tmp/asdf - Function: file.directory - Result: Changed Started: - 14:31:54.018710 Duration: 0.787 ms
  Name: /tmp/asdf/asdf - Function: file.directory - Result: Changed Started: - 14:31:54.019846 Duration: 0.832 ms
  Name: /tmp/asdf/asdf/asdf - Function: file.managed - Result: Changed Started: - 14:31:54.020933 Duration: 1.0 ms
----------
          ID: step3
    Function: git.latest
        Name: https://github.com/saltstack/salt
      Result: False
     Comment: Clone failed: fatal: could not create work tree dir '/tmp/asdf/asdf': Permission denied
     Started: 14:31:54.045517
    Duration: 1799.632 ms
     Changes:   
              ----------
              forced clone:
                  True

Summary for ubuntu1
------------
Succeeded: 5 (changed=4)
Failed:    1
------------
Total states run:     6
Total run time:   1.868 s

New Behavior

ubuntu1:
  Name: git - Function: pkg.installed - Result: Clean Started: - 14:34:23.310340 Duration: 58.718 ms
  Name: asdf - Function: user.present - Result: Clean Started: - 14:34:23.371201 Duration: 8.656 ms
  Name: /tmp/asdf - Function: file.directory - Result: Changed Started: - 14:34:23.381185 Duration: 0.752 ms
  Name: /tmp/asdf/asdf - Function: file.directory - Result: Changed Started: - 14:34:23.382278 Duration: 0.777 ms
  Name: /tmp/asdf/asdf/asdf - Function: file.managed - Result: Changed Started: - 14:34:23.383309 Duration: 1.013 ms
  Name: https://github.com/saltstack/salt - Function: git.latest - Result: Changed Started: - 14:34:23.396015 Duration: 57662.77 ms

Summary for ubuntu1
------------
Succeeded: 6 (changed=4)
Failed:    0
------------
Total states run:     6
Total run time:  57.733 s

Tests written?

No

Given how the issue remained untouched for over a year, I wouldn't expect the fix to be trivial, so I'm welcoming any feedback related to this issue.

@ghost
Copy link

ghost commented Apr 4, 2017

@zer0def, thanks for your PR! By analyzing the history of the files in this pull request, we identified @terminalmage, @mafrosis and @ffa to be potential reviewers.

@cachedout
Copy link
Contributor

@terminalmage Can you please have a look at this?

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