Port #51625 to master #56953

alexey-zhukovin · 2020-04-28T19:50:25Z

What does this PR do?

Port #51625 to master

valentin2105 · 2020-08-11T04:06:36Z

Any idea when this will get merged ? Can I help ? @alexey-zhukovin

waynew · 2021-03-11T17:16:23Z

@valentin2105 Hey, are you still interested/able to help with this? Currently the problem is that there aren't any tests written for this. If you're able to help, writing tests would be appreciated! If you need help getting started with tests, I run a test clinic on Tues/Thurs (check https://saltproject.io/ for more on times)

max-arnold · 2022-03-17T17:30:28Z

@alexey-zhukovin Hey! Do you mind adding the classic=False keyword arg to snap.installed and snap.install, that will add the --classic CLI option?

Quite useful to install microk8s

myii · 2022-03-20T19:03:51Z

salt/states/snap.py

+        ret["comment"] = 'Failed to switch Package "{0}" to channel {1}'.format(
+            name, channel
+        )
+        ret["comment"] += "\noutput:\n" + install["output"]


I've been testing out using these modules in the packages-formula, to replace the use of cmd.run for snap installations. Hit a traceback with this line:

An exception occurred in this state: Traceback (most recent call last): File "/usr/lib/python3/dist-packages/salt/state.py", line 2179, in call ret = self.states[cdata["full"]]( File "/usr/lib/python3/dist-packages/salt/loader/lazy.py", line 149, in __call__ return self.loader.run(run_func, *args, **kwargs) File "/usr/lib/python3/dist-packages/salt/loader/lazy.py", line 1201, in run return self._last_context.run(self._run_as, _func_or_method, *args, **kwargs) File "/usr/lib/python3/dist-packages/salt/loader/lazy.py", line 1216, in _run_as return _func_or_method(*args, **kwargs) File "/usr/lib/python3/dist-packages/salt/loader/lazy.py", line 1249, in wrapper return f(*args, **kwargs) File "/var/cache/salt/minion/extmods/states/snap.py", line 84, in installed ret["comment"] += "\noutput:\n" + install["output"] UnboundLocalError: local variable 'install' referenced before assignment

Looking at the code, realised that this line should actually be:

Suggested change

ret["comment"] += "\noutput:\n" + install["output"]

ret["comment"] += "\noutput:\n" + refresh["output"]

Left over from a copy-paste of L53 above, perhaps.

@lordcirth As the original PR author, would you mind confirming this suggestion?

@myii Sounds correct to me

Appreciate the response, @lordcirth. I've got another question, if you don't mind.

I've found that the execution module functions are returning bytes instead of string objects (where relevant). Should __salt__["cmd.run_all"]() be used instead of subprocess.check_output()? Checking across other modules, that seems to be the main method being used when working with command output.

I'm not very experienced with Python or Saltstack internals. Your guess is probably better than mine.

@lordcirth OK, If I get around to pushing this to the packages-formula, I'll try to refactor it accordingly. I'll report back here on how things work out. Thanks again for the prompt response -- and for providing these modules in the first place!

max-arnold · 2022-03-21T04:00:37Z

Patch to support the --classic option:

diff -Naur salt.orig/modules/snap.py salt/modules/snap.py
--- salt.orig/modules/snap.py	2022-03-21 10:56:38.000000000 +0700
+++ salt/modules/snap.py	2022-03-21 10:58:48.000000000 +0700
@@ -28,7 +28,7 @@
     )
 
 
-def install(pkg, channel=None, refresh=False):
+def install(pkg, channel=None, refresh=False, classic=False):
     """
     Install the specified snap package from the specified channel.
     Returns a dictionary of "result" and "output".
@@ -42,6 +42,9 @@
     refresh : False
         If True, use "snap refresh" instead of "snap install".
         This allows changing the channel of a previously installed package.
+
+    classic : False
+        Install a classic snap package if True
     """
     args = []
     ret = {"result": None, "output": ""}
@@ -54,6 +57,9 @@
     if channel:
         args.append("--channel=" + channel)
 
+    if classic:
+        args.append("--classic")
+
     try:
         # Try to run it, merging stderr into output
         ret["output"] = subprocess.check_output(
diff -Naur salt.orig/states/snap.py salt/states/snap.py
--- salt.orig/states/snap.py	2022-03-21 10:57:50.000000000 +0700
+++ salt/states/snap.py	2022-03-21 10:58:34.000000000 +0700
@@ -24,7 +24,7 @@
     )
 
 
-def installed(name, channel=None):
+def installed(name, channel=None, classic=False):
     """
     Ensure that the named snap package is installed
 
@@ -33,6 +33,9 @@
 
     channel
         Optional. The channel to install the package from.
+
+    classic
+        Optional. Install a classic snap package if True
     """
     ret = {"name": name, "changes": {}, "pchanges": {}, "result": None, "comment": ""}
 
@@ -45,7 +48,7 @@
             ret["result"] = None
             return ret
 
-        install = __salt__["snap.install"](name, channel=channel)
+        install = __salt__["snap.install"](name, channel=channel, classic=classic)
         if install["result"]:
             ret["comment"] = 'Package "{0}" was installed'.format(name)
             ret["changes"]["new"] = name
@@ -73,7 +76,7 @@
             return ret
 
         refresh = __salt__["snap.install"](
-            name, channel=channel, refresh=True
+            name, channel=channel, refresh=True, classic=classic
         )
         if refresh["result"]:
             ret["comment"] = 'Package "{0}" was switched to channel {1}'.format(

max-arnold · 2022-03-22T11:04:43Z

There are some encoding issues here:

   "comment": "An exception occurred in this state: Traceback (most recent call last):\n  File \"/usr/lib/python3/dist-packages/salt/state.py\", line 2153, in call\n    ret = self.states[cdata[\"full\"]](\n  File \"/usr/lib/python3/dist-packages/salt/loader.py\", line 2106, in wrapper\n    return f(*args, **kwargs)\n  File \"/var/cache/salt/minion/extmods/states/snap.py\", line 60, in installed\n    ret[\"comment\"] += \"\\noutput:\\n\" + install[\"output\"]\nTypeError: can only concatenate str (not \"bytes\") to str\n",

As suggested by @myii, going to try using cmd.run_all instead of subprocess.check_output

max-arnold · 2022-03-22T12:59:04Z

salt.zip

Added the --classic option
Removed obsolete pchanges usage
Hopefully fixed the unicode issue

Overall it needs more testing and polishing (e.g. proper logging), because the fixes I made were quick and dirty (and I'm not familiar with snap)

max-arnold · 2022-03-23T04:56:32Z

salt.zip

Fixed another issue reported by Imran (via Slack):

sudo snap list
Name        Version   Rev    Tracking       Publisher              Notes
core18      20220309  2344   latest/stable  canonical✓             base
core20      20220304  1376   latest/stable  canonical✓             base
lxd         4.0.9     22526  4.0/stable/…   canonical✓             -
microk8s    v1.23.4   3021   1.23/stable    canonical✓             classic
powershell  7.2.2     202    latest/stable  microsoft-powershell✓  classic
snapd       2.54.4    15177  latest/stable  canonical✓             snapd

The channel Tracking field can be truncated (see the lxd package). As suggested, I replaced "snap list" with "snap info". This doesn't solve a use-case when multiple versions of a package are installed https://forum.snapcraft.io/t/parallel-installs/7679, but fixes the truncation issue.

And overall I think that multiple package managers in a single OS are breaking the pkg state abstraction layer. It is no longer possible to use a single pkg.installed state when packages can be installed either using apt-get or snap. We have to resort to the separate snap.installed state to manage snap packages.

twangboy

This needs a changelog and a testcase. Also needs to have the feedback implemented.

whytewolf · 2023-08-31T19:47:25Z

until the pre-commit is fixed, tests are added, and a changelog added. this PR can not be merged.

dwoz · 2023-12-10T09:13:38Z

Closing this due to inactivity. Anyone should feel free to re-open it if they want to see it through to the end in one release cycle.

lkubb · 2024-09-26T23:42:51Z

For anyone that followed this, I just released a Salt extension for Snapd: https://github.com/salt-extensions/saltext-snap

It should be much more comprehensive than the previous unported modules since it interfaces with the REST API in addition to the CLI, depending on the query/operation, and offers several states in addition to installed/removed (such as connected and service_running with mod_watch support).

Port saltstack#51625 to master

a2d3900

alexey-zhukovin added Needs-Testcase PR needs test cases written, or the issue is about a bug/feature that needs test cases master-port labels Apr 28, 2020

alexey-zhukovin requested a review from a team as a code owner April 28, 2020 19:50

ghost requested review from twangboy and removed request for a team April 28, 2020 19:50

dwoz added the has-failing-test label May 13, 2020

dwoz removed the has-failing-test label Jun 16, 2020

garethgreenaway mentioned this pull request Aug 7, 2020

[BUG] Snap.installed not avalable in 3001 #58132

Open

Merge branch 'master' into master-port-51625

ffec749

dwoz added the has-failing-test label Sep 22, 2020

Merge branch 'master' into master-port-51625

b4ae344

myii suggested changes Mar 20, 2022

View reviewed changes

myii mentioned this pull request Apr 10, 2022

[FEATURE] Use saltstack/salt proposed snap modules to improve support in this formula (replace cmd.run states) saltstack-formulas/packages-formula#87

Open

twangboy requested changes Oct 28, 2022

View reviewed changes

twangboy added the needs-changelog-file label Oct 28, 2022

joshmcorreia mentioned this pull request Feb 23, 2023

support for ubuntu snappy #27007

Closed

Merge branch 'master' into master-port-51625

763ef73

whytewolf temporarily deployed to ci August 31, 2023 19:55 — with GitHub Actions Inactive

whytewolf temporarily deployed to ci August 31, 2023 21:30 — with GitHub Actions Inactive

whytewolf temporarily deployed to ci August 31, 2023 21:55 — with GitHub Actions Inactive

dwoz closed this Dec 10, 2023

dwoz added help-wanted Community help is needed to resolve this Abandoned labels Dec 10, 2023

dwoz temporarily deployed to ci December 10, 2023 09:45 — with GitHub Actions Inactive

dwoz temporarily deployed to ci December 10, 2023 22:26 — with GitHub Actions Inactive

dwoz temporarily deployed to ci December 10, 2023 22:27 — with GitHub Actions Inactive

dwoz temporarily deployed to ci December 11, 2023 05:38 — with GitHub Actions Inactive

dwoz temporarily deployed to ci December 11, 2023 05:39 — with GitHub Actions Inactive

dwoz temporarily deployed to ci December 11, 2023 05:41 — with GitHub Actions Inactive

dwoz temporarily deployed to ci December 11, 2023 05:42 — with GitHub Actions Inactive

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Port #51625 to master #56953

Port #51625 to master #56953

alexey-zhukovin commented Apr 28, 2020 •

edited by garethgreenaway

Loading

valentin2105 commented Aug 11, 2020 •

edited

Loading

waynew commented Mar 11, 2021

max-arnold commented Mar 17, 2022

myii Mar 20, 2022

lordcirth Mar 20, 2022

myii Mar 21, 2022

lordcirth Mar 21, 2022

myii Mar 21, 2022

max-arnold commented Mar 21, 2022

max-arnold commented Mar 22, 2022

max-arnold commented Mar 22, 2022 •

edited

Loading

max-arnold commented Mar 23, 2022

twangboy left a comment •

edited

Loading

whytewolf commented Aug 31, 2023

dwoz commented Dec 10, 2023

lkubb commented Sep 26, 2024

	ret["comment"] += "\noutput:\n" + install["output"]
	ret["comment"] += "\noutput:\n" + refresh["output"]

Port #51625 to master #56953

Port #51625 to master #56953

Conversation

alexey-zhukovin commented Apr 28, 2020 • edited by garethgreenaway Loading

What does this PR do?

valentin2105 commented Aug 11, 2020 • edited Loading

waynew commented Mar 11, 2021

max-arnold commented Mar 17, 2022

myii Mar 20, 2022

Choose a reason for hiding this comment

lordcirth Mar 20, 2022

Choose a reason for hiding this comment

myii Mar 21, 2022

Choose a reason for hiding this comment

lordcirth Mar 21, 2022

Choose a reason for hiding this comment

myii Mar 21, 2022

Choose a reason for hiding this comment

max-arnold commented Mar 21, 2022

max-arnold commented Mar 22, 2022

max-arnold commented Mar 22, 2022 • edited Loading

max-arnold commented Mar 23, 2022

twangboy left a comment • edited Loading

Choose a reason for hiding this comment

whytewolf commented Aug 31, 2023

dwoz commented Dec 10, 2023

lkubb commented Sep 26, 2024

alexey-zhukovin commented Apr 28, 2020 •

edited by garethgreenaway

Loading

valentin2105 commented Aug 11, 2020 •

edited

Loading

max-arnold commented Mar 22, 2022 •

edited

Loading

twangboy left a comment •

edited

Loading