Guard boto3_elasticsearch loading properly

[oeuftete]

What does this PR do?

Fixes this traceback on module loading, bringing boto3_elasticsearch into line with other boto3 modules.

2020-09-25 14:27:37,230 [salt.loader      :1764][DEBUG   ][5970] Error loading module.boto3_elasticsearch: __init__ failed
Traceback (most recent call last):
  File "/usr/lib/python3.6/site-packages/salt/loader.py", line 1754, in _load_module
    module_init(self.opts)
  File "/usr/lib/python3.6/site-packages/salt/modules/boto3_elasticsearch.py", line 96, in __init__
    __utils__["boto3.assign_funcs"](__name__, "es")
  File "/usr/lib/python3.6/site-packages/salt/loader.py", line 1278, in __getitem__
    func = super(LazyLoader, self).__getitem__(item)
  File "/usr/lib/python3.6/site-packages/salt/utils/lazy.py", line 108, in __getitem__
    raise KeyError(key)
KeyError: 'boto3.assign_funcs'

What issues does this PR fix or reference?

Fixes (partially): #55848

Merge requirements satisfied?

• (n/a) Docs
• Changelog - https://docs.saltstack.com/en/master/topics/development/changelog.html
• Tests written/updated

If tests are required for this, I need guidance.

[waynew]

Should this have a virtual, using the HAS_BOTO?

[oeuftete]

Should this have a virtual, using the HAS_BOTO?

🤷‍♂️

I'm mostly thoughtlessly following the pattern of similar modules, e.g.:

https://github.com/saltstack/salt/blob/ee75b13cc0dce03c9964969c22688710f3346502/salt/modules/boto_cloudwatch_event.py#L77-L87

[oeuftete]

It looks like the pre-commit formatting somehow broke the tests.

Traceback (most recent call last):
  File "/tmp/kitchen/testing/tests/unit/modules/test_boto3_elasticsearch.py", line 399, in test_add_tags_error
    "testdomain", tags={"foo": "bar", "baz": "qux"}, **CONN_PARAMETERS
  File "/tmp/kitchen/testing/salt/modules/boto3_elasticsearch.py", line 152, in add_tags
    "TagList": [{"Key": k, "Value": value} for k, value in tags or {}.items()],
  File "/tmp/kitchen/testing/salt/modules/boto3_elasticsearch.py", line 152, in <listcomp>
    "TagList": [{"Key": k, "Value": value} for k, value in tags or {}.items()],
ValueError: too many values to unpack (expected 2)

Edit, what had to be fixed:

diff --git a/salt/modules/boto3_elasticsearch.py b/salt/modules/boto3_elasticsearch.py
index 3bffa5b9a2..f6ff673434 100644
--- a/salt/modules/boto3_elasticsearch.py
+++ b/salt/modules/boto3_elasticsearch.py
@@ -149,7 +149,9 @@ def add_tags(
     if arn:
         boto_params = {
             "ARN": arn,
-            "TagList": [{"Key": k, "Value": value} for k, value in tags or {}.items()],
+            "TagList": [
+                {"Key": k, "Value": value} for k, value in tags.items() or {}.items()
+            ],
         }
         try:
             conn = _get_conn(region=region, key=key, keyid=keyid, profile=profile)

[waynew]

Suggested change

	{"Key": k, "Value": value} for k, value in tags.items() or {}.items()
	{"Key": k, "Value": value} for k, value in (tags or {}).items()

Yeah, that pre-commit horked this line up completely. Something seems off with the indention here? but... this is the logic fix.

[waynew]

cc @s0undt3ch

[oeuftete]

I think what's in the PR right now is logically ok, but yours is clearer, I'll add that.

What the pre-commit did originally was change six.iteritems(tags or {}) to tags or {}.items(), which is definitely not ok!

[waynew]

what's in the PR now will fail when tags is None:

>>> tags = None
>>> tags.items() or {}.items()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: 'NoneType' object has no attribute 'items'

That was an implicit fallback with iteritems(tags or {}) 🙃

The other option would be tags.items() if tags else {}.items() but that would be silly 😂

[oeuftete]

🤦

[oeuftete]

I am really confused now. How did this manage to get in to 3002 in d089e81/#58622? The reason I saw and fixed this (not quite correctly) in the first place is that it was failing a test. How did that not happen for the 3002 release?

[oeuftete]

Ah, I see, the comprehension was changed: https://github.com/saltstack/salt/pull/58622/files#diff-fe8071ebcb4727856197b666dbf829930088a5d5fc5d8c82a0d806d03ecf059bR149

[waynew]

That comprehension needs some fixing.

I don't have strong feelings about tests here, but you could easily write two tests that cover both HAS_BOTO and not HAS_BOTO.

I still think that we want virtual to be returning false without boto though, right? If we don't have boto, we can't connect to elasticsearch... because we don't have boto?

[waynew]

oooh, I see what you mean. That check boto reqs probably returns false without boto 👍

[oeuftete]

bad rebase, fixing

[ScriptAutomate]

Is this issue the same one being experienced here:

• swarm module init exception during lazyload #55187

I had initially added to the conversation in this issue because of all of these traceback error outputs in the debug log for pkg.installed, including the one that looks to be getting addressed in this PR

[oeuftete]

Is this issue the same one being experienced here:

• swarm module init exception during lazyload #55187

I had initially added to the conversation in this issue because of all of these traceback error outputs in the debug log for pkg.installed, including the one that looks to be getting addressed in this PR

@ScriptAutomate I think it's best to keep this separate. They're sort of related, in that they are both issues where a not-quite-right interaction between __virtual__ and __init__ leads to a module load failure with traceback, but the issues are independent.

[sagetherage]

@major0 if you want to review or at least know about this PR simply letting you know :)

[Ch3LL]

@waynew can you re-review here