added support for freebsd_update #36675

[mamalos]

What does this PR do?

It basically adds a salt wrapper to FreeBSD's freebsd-update command.

What issues does this PR fix or reference?

#36675

New Behavior

People will be able to user salt '*' freebsd-update.xxx to mimic freebsd-update behaviour.

Tests written?

No

[mamalos]

All my basic functions accept the same arguments. An example follows:

def fetch(pre='', post='', err_=None, run_args={}, **kwargs):

The basic reason I've chosen to do so, is that they are all --except for the not-yet-implemented upgrade function-- calling the same function: _wrapper(), which accepts the exact same arguments. This way, even though most of these functions in their current implementation don't use most of their arguments, if someone in the future wanted to exploit the arguments' existence, their new code would be backwards compatible.

[cachedout]

There are a few minor things to fix up here but overall it looks great. Please have a look and let me know what you think. Thanks!

[cachedout]

We'll need a import salt.ext.six as six here.

[cachedout]

Could we change this to six.iteritems(res)?

[cachedout]

six.iteritems(stdout) here as well

[cachedout]

We can't have mutable types as a default. (Explanation: http://reinout.vanrees.org/weblog/2012/04/18/default-parameters.html)

[cachedout]

Same issue with mutable arg here.

[cachedout]

We also like to avoid **kwargs wherever possible in execution modules.

[mamalos]

I think (as explained in the pull request) that it could be safely be kept as is, since it reduces code complexity and maintenance.

[cachedout]

Same issue here with mutable arg and **kwargs

[cachedout]

There are some additional issues which require attention here: https://jenkins.saltstack.com/job/PR/job/salt-pr-lint-n/5987/violations/file/salt/modules/freebsd_update.py/

[mamalos]

@cachedout, I'm sorry! I didn't see your comments and made some changes on my branch, which I'm not sure that they have been automatically updated in my pull request, and that don't contain the corrections you suggested. Moreover, I must have done something wrong, because my initial build failed (and I wasn't able to understand why).

The good thing is that my new changes -accidentally- fix some of the things you suggested :). I'll try to fix the rest of them, but there's a design problem I'm facing: I'm using **kwargs because it removed lots of boilerplate (otherwise all my functions' prototypes would have to accept the same keyword arguments along with their docstrings, and if a later argument appeared in freebsd-update, all these functions' prototypes would have to be edited to support it). If you see the code, you'll see that **kwargs usage is quite safe; it is only evaluated once in _cmd() function and each supported keyword argument is fetched if present. So, I think I'll keep it.

Moreover, I'll change my _cmd() function to get the keyword arguments' values from the dict instead of checking if their key exist and then fetch their value.

PS. I'll submit a pull request for making the memoize decorator support keyword arguments caching as well, and then I'll have to remember to use it in my _cmd() function.

[mamalos]

Hmm...@cachedout, if you could please help me understand why my builds fail, I would be really grateful! Because when I try to see the "Details", I can see some huge logfiles, where I can't discern which part of my code causes jenkins to fail.

[cachedout]

Hi @mamalos. Something went wrong here. You may have pushed from the wrong branch since it seems like a number of unrelated commits are present. It might be easier to have you close this and re-open a PR against the correct branch. Let me know if I can help.

[mamalos]

Hey @cachedout. Yes I need some help. I'll tell you what I've been doing and please tell me when I made the mistake. The following code assumes that I've already forked salt, cloned it locally, created a new branch, committed changes, fetched upstream, rebased, pushed and started a pull request. Then, some time has passed and I want to make a new branch for a new pull request:

$ git fetch upstream
$ git  checkout develop
$ git merge upstream develop # I wanted to update my origin's develop branch
$ git push                                  # so this is how I thought I'd do it
$ git commit -b  adding-freebsdupdate-support upstream/develop
$ # made various changes to add freebsd-update support
$ git commit -a -m "added freebsd-update support"
$ git fetch upstream
$ git rebase upstream/develop adding-freebsdupdate-support
$ git push -u origin adding-freebsdupdate-support

That's how I did it on the first pull request for update-freebsd support, which failed in many tests.

Then I tried to make the changes you suggested by executing:

$ # vi freebsd_update.py and making changes
$ git commit -a -m "corrections blahblah"
$ git fetch upstream
$ git rebase upstream/develop adding-freebsdupdate-support
$ git push -u origin adding-freebsdupdate-support

and after the last command (push) I got the following error:

To https://github.com/mamalos/salt
 ! [rejected]        adding-freebsdupdate-support -> adding-freebsdupdate-support (non-fast-forward)
error: failed to push some refs to 'https://github.com/mamalos/salt'
hint: Updates were rejected because the tip of your current branch is behind
hint: its remote counterpart. Integrate the remote changes (e.g.
hint: 'git pull ...') before pushing again.
hint: See the 'Note about fast-forwards' in 'git push --help' for details.

At this point I tried to pull the specific branch and push back the merged changes (from this point onwards I have the terminal history so I'll also paste the output):

$ git pull origin adding-freebsdupdate-support
$ vi salt/modules/freebsd_update.py # It was as if my local branch was not aware of my previous commits, so basically I had to merge all my changes by hand
$ git commit -a -m "changed prototypes to become simpler"
[adding-freebsdupdate-support eafb1c2] changed prototypes to become simpler
git push -u origin adding-freebsdupdate-support
Counting objects: 714, done.
Delta compression using up to 8 threads.
Compressing objects: 100% (344/344), done.
Writing objects: 100% (714/714), 135.24 KiB | 0 bytes/s, done.
Total 714 (delta 533), reused 522 (delta 362)
remote: Resolving deltas: 100% (533/533), completed with 216 local objects.
To https://github.com/mamalos/salt
   cf46b75..eafb1c2  adding-freebsdupdate-support -> adding-freebsdupdate-support
Branch adding-freebsdupdate-support set up to track remote branch adding-freebsdupdate-support from origin.

And that's the code for the last test that failed. After that I worked in a new branch (following the steps of the contributing guide), made a pull request and my pull request was merged (by you:)). But, before following the steps, I did:

$ git checkout develop
$ git merge remotes/origin/develop
Already up-to-date.

And then:

$ git checkout -b add-keyword-arguments-support-to-decorators-memoize upstream/develop
Branch add-keyword-arguments-support-to-decorators-memoize set up to track remote branch develop from upstream.
Switched to a new branch 'add-keyword-arguments-support-to-decorators-memoize'
$ git commit -a -m "added support for keyword arguments (**kwargs) caching in decorators.memoize #36970"
[add-keyword-arguments-support-to-decorators-memoize 79f2dc6] added support for keyword arguments (**kwargs) caching in decorators.memoize #36970
 1 file changed, 10 insertions(+), 4 deletions(-)
$ git fetch upstream
$ git rebase upstream/develop add-keyword-arguments-support-to-decorators-memoizeCurrent branch add-keyword-arguments-support-to-decorators-memoize is up to date.
$ git push -u origin add-keyword-arguments-support-to-decorators-memoize
Counting objects: 229, done.
Delta compression using up to 8 threads.
Compressing objects: 100% (103/103), done.
Writing objects: 100% (229/229), 50.26 KiB | 0 bytes/s, done.
Total 229 (delta 183), reused 161 (delta 123)
remote: Resolving deltas: 100% (183/183), completed with 95 local objects.
To https://github.com/mamalos/salt
 * [new branch]      add-keyword-arguments-support-to-decorators-memoize -> add-keyword-arguments-support-to-decorators-memoize
Branch add-keyword-arguments-support-to-decorators-memoize set up to track remote branch add-keyword-arguments-support-to-decorators-memoize from origin.

So, after reading your last message I:

$ git checkout adding-freebsdupdate-support
$ git fetch upstream
remote: Counting objects: 283, done.
remote: Compressing objects: 100% (2/2), done.
remote: Total 283 (delta 192), reused 192 (delta 192), pack-reused 89
Receiving objects: 100% (283/283), 86.23 KiB | 0 bytes/s, done.
Resolving deltas: 100% (218/218), completed with 95 local objects.
From https://github.com/saltstack/salt
   7dd91c2..39d59ab  2015.8     -> upstream/2015.8
   71fd01a..6b94153  2016.3     -> upstream/2016.3
   908383f..a5c26d4  carbon     -> upstream/carbon
   f54249f..454a721  develop    -> upstream/develop
$ git rebase upstream/develop adding-freebsdupdate-support
First, rewinding head to replay your work on top of it...
Applying: added support for freebsd_update #36675
Applying: added support for freebsd_update #36675
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
No changes -- Patch already applied.
Applying: made function prototypes less complicated
Applying: added cachedout's corrections
$ git push -u origin adding-freebsdupdate-support
To https://github.com/mamalos/salt
 ! [rejected]        adding-freebsdupdate-support -> adding-freebsdupdate-support (non-fast-forward)
error: failed to push some refs to 'https://github.com/mamalos/salt'
hint: Updates were rejected because the tip of your current branch is behind
hint: its remote counterpart. Integrate the remote changes (e.g.
hint: 'git pull ...') before pushing again.
hint: See the 'Note about fast-forwards' in 'git push --help' for details.

...and I'm clearly unsure of what to do now...

[mamalos]

I think I corrected the mistake. I'm giving it a final try and if it fails I'll just start a new PR as you suggested.

[mamalos]

Hmm...it failed again, I'll start a new PR.

[cachedout]

This actually looks close to right now. All we need is for you to fix the issues here in your local checkout:

https://jenkins.saltstack.com/job/PR/job/salt-pr-lint-n/6117/violations/file/salt/modules/freebsd_update.py/

Then, just commit to your local branch and push the change to this branch adding-freebsdupdate-support on your GithHub fork. They will then be automatically reflected in this PR.

[mamalos]

Ah, cool, this commit passed the tests, so I think we're done now! :)

[eradman]

There is one test that complains about missing examples which should probably
receive some attention:

-> integration.modules.sysmod.SysModuleTest.test_valid_docs  ................
    Traceback (most recent call last):
      File "/usr/home/freebsd/git/salt/tests/integration/modules/sysmod.py", line 69, in test_valid_docs
        '\n'.join(['  - {0}'.format(f) for f in sorted(noexample)]),
    AssertionError: There are some functions which do not have a docstring or do not have an example:
    No docstring:

    No example:
      - freebsd-update.fetch
      - freebsd-update.ids
      - freebsd-update.install
      - freebsd-update.rollback
      - freebsd-update.update
      - freebsd-update.upgrade

[mamalos]

@eradman, I agree with you (and the tests) that it would be much wiser if I wrote CLI examples of how the functions should be used (I hadn't noticed that other "public" functions did), so, next week I'll try to find some time and write examples and reissue a pull request.