Skip to content

master-port 49201#54976

Merged
dwoz merged 22 commits intosaltstack:masterfrom
mchugh19:port-49201
Apr 20, 2020
Merged

master-port 49201#54976
dwoz merged 22 commits intosaltstack:masterfrom
mchugh19:port-49201

Conversation

@mchugh19
Copy link
Contributor

@mchugh19 mchugh19 commented Oct 12, 2019

What does this PR do?

Port of develop merged #49201 to master

Tests written?

No - There don't seem to be any tests of recurse_types

dwoz
dwoz requested changes Oct 12, 2019
View reviewed changes
Copy link
Contributor

@dwoz dwoz left a comment

Choose a reason for hiding this comment

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

@mchugh19 Is it possible to add a unit test for this please?

@mchugh19
Copy link
Contributor Author

mchugh19 commented Oct 12, 2019

@mchugh19 Is it possible to add a unit test for this please?

As mentioned, there aren't any tests of recurse_types. Are there any suggestions of how to handle this?

@dwoz dwoz requested a review from a team as a code owner November 20, 2019 07:54
@ghost ghost requested a review from cmcmarrow November 20, 2019 07:54
@dwoz dwoz added the needs-testcase PR needs test cases written, or the issue is about a bug/feature that needs test cases label Dec 3, 2019
@mchugh19
Copy link
Contributor Author

mchugh19 commented Jan 6, 2020

Hey all. Happy to look into tests for this, if there is a recommendation on what they should look like.

@garethgreenaway
Copy link
Contributor

garethgreenaway commented Jan 10, 2020

@mchugh19 Apologies for the delay in responding. We did have some existing tests that were testing including two conflicting recurse options, in the unit test for file.directory. I had started off tweaking things to just provide a quick example to pass along but quickly realized that the test in question was rather broken, so in addition to some cleanup I also added a test to test the use of silent as a recurse option.

@garethgreenaway garethgreenaway removed the needs-testcase PR needs test cases written, or the issue is about a bug/feature that needs test cases label Jan 10, 2020
@codecov
Copy link

codecov bot commented Jan 13, 2020

Codecov Report

Merging #54976 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #54976   +/-   ##
=======================================
  Coverage   39.36%   39.36%           
=======================================
  Files        1495     1495           
  Lines      262788   262788           
  Branches    55570    55570           
=======================================
  Hits       103416   103416           
  Misses     147571   147571           
  Partials    11801    11801
Flag Coverage Δ
#centos7 39.36% <0%> (ø) ⬆️
#m2crypto 39.36% <0%> (ø) ⬆️
#py2 39.36% <0%> (ø) ⬆️
#runtests 39.36% <0%> (ø) ⬆️
#zeromq 39.36% <0%> (ø) ⬆️

1 similar comment
@codecov
Copy link

codecov bot commented Jan 13, 2020

Codecov Report

Merging #54976 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #54976   +/-   ##
=======================================
  Coverage   39.36%   39.36%           
=======================================
  Files        1495     1495           
  Lines      262788   262788           
  Branches    55570    55570           
=======================================
  Hits       103416   103416           
  Misses     147571   147571           
  Partials    11801    11801
Flag Coverage Δ
#centos7 39.36% <0%> (ø) ⬆️
#m2crypto 39.36% <0%> (ø) ⬆️
#py2 39.36% <0%> (ø) ⬆️
#runtests 39.36% <0%> (ø) ⬆️
#zeromq 39.36% <0%> (ø) ⬆️

@garethgreenaway
More cleanup in test_directory test. Shuffling things around to ensur…
e1dc252 
…e the tests pass on Windows. Adding additional patches to reset the mock of file.check_perms back to what is expected since the ret is changed when the module is run.
@garethgreenaway
Merge branch 'master' into port-49201
433d068
Ch3LL
Ch3LL suggested changes Mar 31, 2020
View reviewed changes
@mchugh19
Copy link
Contributor Author

mchugh19 commented Apr 6, 2020

Support output like:

----------
          ID: /tmp/bigthing
    Function: file.directory
      Result: True
     Comment: Directory /tmp/bigthing updated
     Started: 17:15:16.822358
    Duration: 416.771 ms
     Changes:   
              ----------
              recursion:
                  Changes silenced
              user:
                  root

Summary for local
------------
Succeeded: 1 (changed=1)
Failed:    0
------------

where changes is expected to remain a dictionary as later operation will continue to update it.

@mchugh19
Copy link
Contributor Author

mchugh19 commented Apr 15, 2020

re-run pr-pre-commit

@mchugh19
Copy link
Contributor Author

mchugh19 commented Apr 16, 2020

re-run pr-windows2019-py3

@waynew
Copy link
Contributor

waynew commented Apr 17, 2020

@dwoz / @Ch3LL tests are passing now 🎉

@dwoz dwoz merged commit 4a68346 into saltstack:master Apr 20, 2020
@sagetherage sagetherage added the ZRelease-Sodium retired label label May 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dwoz dwoz dwoz approved these changes

@cmcmarrow cmcmarrow Awaiting requested review from cmcmarrow

+1 more reviewer

@Ch3LL Ch3LL Ch3LL requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

@Ch3LL Ch3LL

Labels

master-port ZRelease-Sodium retired label

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@mchugh19 @garethgreenaway @waynew @dwoz @Ch3LL @sagetherage @DmitryKuzmenko