fix boto3_route53 module for Python3 #60951

IndraGunawan · 2021-09-24T13:03:23Z

What does this PR do?

What issues does this PR fix or reference?

Fixes: this PR fixed re.sub behaviour for Python 3

Previous Behavior

              An exception occurred in this state: Traceback (most recent call last):
                File "/usr/lib/python3/dist-packages/salt/modules/boto3_route53.py", line 881, in aws_encode
                  ret = re.sub(r"\\x([a-f0-8]{2})", _hexReplace, x.encode("unicode_escape"))
                File "/usr/lib/python3.6/re.py", line 191, in sub
                  return _compile(pattern, flags).sub(repl, string, count)
              TypeError: cannot use a string pattern on a bytes-like object

              During handling of the above exception, another exception occurred:

              Traceback (most recent call last):
                File "/usr/lib/python3/dist-packages/salt/state.py", line 2172, in call
                  *cdata["args"], **cdata["kwargs"]
                File "/usr/lib/python3/dist-packages/salt/loader.py", line 1241, in __call__
                  return self.loader.run(run_func, *args, **kwargs)
                File "/usr/lib/python3/dist-packages/salt/loader.py", line 2274, in run
                  return self._last_context.run(self._run_as, _func_or_method, *args, **kwargs)
                File "/usr/lib/python3/dist-packages/contextvars/__init__.py", line 38, in run
                  return callable(*args, **kwargs)
                File "/usr/lib/python3/dist-packages/salt/loader.py", line 2289, in _run_as
                  return _func_or_method(*args, **kwargs)
                File "/usr/lib/python3/dist-packages/salt/loader.py", line 2322, in wrapper
                  return f(*args, **kwargs)
                File "/usr/lib/python3/dist-packages/salt/states/boto3_route53.py", line 766, in rr_present
                  profile=profile,
                File "/usr/lib/python3/dist-packages/salt/loader.py", line 1241, in __call__
                  return self.loader.run(run_func, *args, **kwargs)
                File "/usr/lib/python3/dist-packages/salt/loader.py", line 2274, in run
                  return self._last_context.run(self._run_as, _func_or_method, *args, **kwargs)
                File "/usr/lib/python3/dist-packages/contextvars/__init__.py", line 38, in run
                  return callable(*args, **kwargs)
                File "/usr/lib/python3/dist-packages/salt/loader.py", line 2289, in _run_as
                  return _func_or_method(*args, **kwargs)
                File "/usr/lib/python3/dist-packages/salt/modules/boto3_route53.py", line 1027, in get_resource_records
                  ) if next_rr_name else None
                File "/usr/lib/python3/dist-packages/salt/modules/boto3_route53.py", line 888, in aws_encode
                  raise CommandExecutionError(e)
              salt.exceptions.CommandExecutionError: cannot use a string pattern on a bytes-like object

New Behavior

Remove this section if not relevant

Merge requirements satisfied?

[NOTICE] Bug fixes or features added to Salt require tests.

Docs
Changelog - https://docs.saltproject.io/en/master/topics/development/changelog.html
Tests written/updated

Commits signed with GPG?

Yes

welcome · 2021-09-24T13:03:25Z

Hi there! Welcome to the Salt Community! Thank you for making your first contribution. We have a lengthy process for issues and PRs. Someone from the Core Team will follow up as soon as possible. In the meantime, here’s some information that may help as you continue your Salt journey.
Please be sure to review our Code of Conduct. Also, check out some of our community resources including:

There are lots of ways to get involved in our community. Every month, there are around a dozen opportunities to meet with other contributors and the Salt Core team and collaborate in real time. The best way to keep track is by subscribing to the Salt Community Events Calendar.
If you have additional questions, email us at saltproject@vmware.com. We’re glad you’ve joined our community and look forward to doing awesome things with you!

IndraGunawan · 2021-10-06T12:58:50Z

any chances that @krionbsd or @saltstack/team-boto review this PR?

krionbsd · 2021-10-06T13:29:45Z

any chances that @krionbsd or @saltstack/team-boto review this PR?

Any chance to add a test case for these changes in tests/unit/modules/test_boto_route53.py ?

IndraGunawan · 2021-10-08T03:21:33Z

Any chance to add a test case for these changes in tests/unit/modules/test_boto_route53.py ?

test added

anitakrueger · 2021-11-16T12:39:03Z

Looks like this fixes #59755.
What does it take to get this merged?
Our route53 record management has been broken since moving to 3002.

IndraGunawan · 2021-11-29T18:19:13Z

@krionbsd any chance to review this PR?

IndraGunawan · 2021-11-30T08:05:33Z

@krionbsd failed test is not related to this PR

github-actions · 2022-01-24T06:32:05Z

Hi! I'm your friendly PR bot!

You might be wondering what I'm doing commenting here on your PR.

Yes, as a matter of fact, I am...

I'm just here to help us improve the documentation. I can't respond to
questions or anything, but what I can do, I do well!

Okay... so what do you do?

I detect modules that are missing docstrings or "CLI Example" on existing docstrings!
When I was created we had a lot of these. The documentation for these
modules need some love and attention to make Salt better for our users.

So what does that have to do with my PR?

I noticed that in this PR there are some files changed that have some of these
issues. So I'm leaving this comment to let you know your options.

Okay, what are they?

Well, my favorite, is that since you were making changes here I'm hoping that
you would be the most familiar with this module and be able to add some other
examples or fix any of the reported issues.

If I can, then what?

Well, you can either add them to this PR or add them to another PR. Either way is fine!

Well... what if I can't, or don't want to?

That's also fine! We appreciate all contributions to the Salt Project. If you
can't add those other examples, either because you're too busy, or unfamiliar,
or you just aren't interested, we still appreciate the contributions that
you've made already.

Whatever approach you decide to take, just drop a comment here letting us know!

Detected Issues (click me)

Check Known Missing Docstrings...........................................Failed
- hook id: invoke
- duration: 1.55s
- exit code: 1
The function 'aws_encode' on 'salt/modules/boto3_route53.py' does not have a 'CLI Example:' in it's docstring

Found 1 errors

Thanks again!

waynew

I don't know if it's worth blocking this PR on any of my comments. If you want to make those changes, then awesome! Otherwise I think just having the record of desired changes is good enough 👍

waynew · 2022-01-24T15:27:25Z

tests/unit/modules/test_boto3_route53.py

+def __virtual__():
+    """
+    Returns True/False boolean depending on if Boto3 is installed and correct
+    version.
+    """
+    if not HAS_BOTO3:
+        return False
+    if LooseVersion(boto3.__version__) < LooseVersion(REQUIRED_BOTO3_VERSION):
+        return (
+            False,
+            "The boto3 module must be greater or equal to version {}".format(
+                REQUIRED_BOTO3_VERSION
+            ),
+        )
+    return True


suggestion __virtual__ only applies to Salt modules/states, not tests.

waynew · 2022-01-24T15:28:56Z

tests/unit/modules/test_boto3_route53.py

+}
+
+
+@skipIf(HAS_BOTO3 is False, "The boto module must be installed.")


suggestion can just use the module's HAS_BOTO3 - either skipIf(boto3_route53.HAS_BOTO3... or import HAS_BOTO3 from the module

waynew · 2022-01-24T15:33:25Z

tests/unit/modules/test_boto3_route53.py

+            random.choice(string.ascii_lowercase + string.digits) for _ in range(50)
+        )
+
+        self.conn = MagicMock()


suggestion(non-blocking) create_autospec is preferred (or patch(..., autospec=True)) to a bare MagicMock, since it will get you the correct signature. Sometimes it's not appropriate, though.

waynew · 2022-01-24T15:35:04Z

tests/unit/modules/test_boto3_route53.py

+                [
+                    {
+                        "Name": "blog.saltstack.furniture.",
+                        "ResourceRecords": [{"Value": "127.0.0.1"}],
+                        "TTL": 60,
+                        "Type": "A",
+                    }
+                ],


praise for tests we love to see hard coded expected values.

welcome · 2022-01-25T18:20:32Z

Congratulations on your first PR being merged! 🎉

IndraGunawan requested a review from a team as a code owner September 24, 2021 13:03

IndraGunawan requested review from krionbsd and removed request for a team September 24, 2021 13:03

IndraGunawan force-pushed the fix-binarysearch branch from d259b23 to c9b1de4 Compare October 7, 2021 11:16

anitakrueger mentioned this pull request Nov 16, 2021

[BUG] 3002.5 String encoding error in boto3_route53 #59755

Closed

IndraGunawan added 5 commits November 30, 2021 00:30

fix re.sub for Python 3

c38710a

fix _aws_decode

dc8d379

add test

3ba34fa

fix lint

5a617bc

add _ prefix to aws_encode method

a6d2d69

IndraGunawan force-pushed the fix-binarysearch branch from 56e3928 to a6d2d69 Compare November 29, 2021 16:36

krionbsd approved these changes Nov 29, 2021

View reviewed changes

Merge branch 'master' into fix-binarysearch

8bb7b38

waynew approved these changes Jan 24, 2022

View reviewed changes

garethgreenaway merged commit bcca1e9 into saltstack:master Jan 25, 2022

IndraGunawan deleted the fix-binarysearch branch January 26, 2022 10:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix boto3_route53 module for Python3 #60951

fix boto3_route53 module for Python3 #60951

Uh oh!

IndraGunawan commented Sep 24, 2021 •

edited

Loading

Uh oh!

welcome bot commented Sep 24, 2021

Uh oh!

IndraGunawan commented Oct 6, 2021

Uh oh!

krionbsd commented Oct 6, 2021

Uh oh!

IndraGunawan commented Oct 8, 2021

Uh oh!

anitakrueger commented Nov 16, 2021

Uh oh!

IndraGunawan commented Nov 29, 2021

Uh oh!

IndraGunawan commented Nov 30, 2021

Uh oh!

github-actions bot commented Jan 24, 2022

Uh oh!

waynew left a comment

Uh oh!

waynew Jan 24, 2022

Uh oh!

waynew Jan 24, 2022

Uh oh!

waynew Jan 24, 2022

Uh oh!

waynew Jan 24, 2022

Uh oh!

welcome bot commented Jan 25, 2022

Uh oh!

Uh oh!

		}


		@skipIf(HAS_BOTO3 is False, "The boto module must be installed.")

fix boto3_route53 module for Python3 #60951

fix boto3_route53 module for Python3 #60951

Uh oh!

Conversation

IndraGunawan commented Sep 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

What issues does this PR fix or reference?

Previous Behavior

New Behavior

Merge requirements satisfied?

Commits signed with GPG?

Uh oh!

welcome bot commented Sep 24, 2021

Uh oh!

IndraGunawan commented Oct 6, 2021

Uh oh!

krionbsd commented Oct 6, 2021

Uh oh!

IndraGunawan commented Oct 8, 2021

Uh oh!

anitakrueger commented Nov 16, 2021

Uh oh!

IndraGunawan commented Nov 29, 2021

Uh oh!

IndraGunawan commented Nov 30, 2021

Uh oh!

github-actions bot commented Jan 24, 2022

Hi! I'm your friendly PR bot!

Uh oh!

waynew left a comment

Choose a reason for hiding this comment

Uh oh!

waynew Jan 24, 2022

Choose a reason for hiding this comment

Uh oh!

waynew Jan 24, 2022

Choose a reason for hiding this comment

Uh oh!

waynew Jan 24, 2022

Choose a reason for hiding this comment

Uh oh!

waynew Jan 24, 2022

Choose a reason for hiding this comment

Uh oh!

welcome bot commented Jan 25, 2022

Uh oh!

Uh oh!

IndraGunawan commented Sep 24, 2021 •

edited

Loading