[Resolve #870] Bugfix in _call_sceptre_handler

[alex-harvey-z3q]

Before this, the _call_sceptre_handler method makes multiple calls to
os.getcwd() in determining paths to add and then later paths to remove.
In the event that the sceptre_handler code then makes a call to
os.chdir, the second call ends up removing the wrong path.

This patch ensures that the paths removed are the same ones that are
added.

PR Checklist

• Wrote a good commit message & description [see guide below].
• Commit message starts with [Resolve #issue-number].
• Added/Updated unit tests.
• Added/Updated integration tests (if applicable).
• All unit tests (make test) are passing.
• Used the same coding conventions as the rest of the project.
• The new code passes flake8 (make lint) checks.
• The PR relates to only one subject with a clear title.
  and description in grammatically correct, complete sentences.

Approver/Reviewer Checklist

• Before merge squash related commits.

Other Information

Guide to writing a good commit

[nabeelamjad]

Could you add a regression test that breaks without this PR?

[alex-harvey-z3q]

@nabeelamjad , I have spent 1.5 hours or so on it and I still can't figure out a way to test my change or get my head around those tests. I tried copying the VPC test and adding an os.chdir call in the sceptre handler and that unfortunately breaks many tests. Any suggestions?

[nabeelamjad]

Perhaps something like this?

def test_body_with_chdir_template(self):
    self.template.sceptre_user_data = None
    self.template.name = "chdir"

    current_dir = os.getcwd()

    self.template.path = os.path.join(
        os.getcwd(),
        "tests/fixtures/templates/chdir.py"
    )

    try:
        json.loads(self.template.body)
    except ValueError:
        assert False
    finally:
        os.chdir(current_dir)

Your chdir.py would just contain

# -*- coding: utf-8 -*-

from troposphere import Template

from os import chdir

def sceptre_handler(sceptre_user_data):
    t = Template()

    chdir("..")

    return t.to_json()

I think the issue you're facing is that since you change directory and the tests are using os.getcwd() (which then becomes incorrect as it hasn't been restored).

Alternatively you could use something like this at the top (before the TestTemplate class) so the code is executed before every test in that file. In this case you wouldn't need the restore chdir logic in your new test. (I'm not sure if the code after yield is executed in case of a raise, you may have to wrap it in a try/catch).

@pytest.fixture(autouse=True)
def run_before_each():
    current_dir = os.getcwd()
    
    yield
    
    os.chdir(current_dir)

[alex-harvey-z3q]

@nabeelamjad thanks, that worked perfectly and makes sense. I've updated the PR with the passing tests and confirmed they reproduced the original issue without the bugfix.

[alex-harvey-z3q]

@nabeelamjad , also is there chance I can get my bugfix released after it's merged?

[nabeelamjad]

@alexharv074 I don't have the authority to merge/release. @ngfgrant handles those.

From my end the PR looks good, no issues and a good catch 👍

[alex-harvey-z3q]

@ngfgrant , I saw a comment saying something about linting not passing but I can't find that comment. Was it deleted? AFAICT, linting is fine:

▶ make lint          
flake8 .
/Users/alexharvey/git/home/sceptre/venv/lib/python3.7/site-packages/pycodestyle.py:113: FutureWarning: Possible nested set at position 1
  EXTRANEOUS_WHITESPACE_REGEX = re.compile(r'[[({] | []}),;:]')

Is there anything else?

[ngfgrant]

Hey @alexharv074 i didn’t see a comment about lint. What happens when you ‘make lint’ locally?

[alex-harvey-z3q]

@ngfgrant , nevermind I saw something about it in my email but make lint tests pass locally in the pipeline so I can't see any lint issue.