opsmill / infrahub-sdk-python

Apache License 2.0
3 stars 1 forks source link

bug: InfrahubTransform .git property is broken #49

Open PhillSimonds opened 2 weeks ago

PhillSimonds commented 2 weeks ago

Component

InfrahubTransform

Infrahub SDK version

0.13.1

Current Behavior

When an InfrahubTransform is instantiated it's .git attribute is undefined. The branch_name property of the InfrahubTransform tries to access .git and instantiate it if it does not exist.

        if not self.git:
            self.git = Repo(self.root_directory)

This block of code raises an error because the InfrahubTransform object does not have a .git attribute.

Expected Behavior

I believe the intent is that we create both .git in the event it is undefined (e.g. not an attribute of InfrahubTransform) and in the case it is set to None, not only in the case it is set to none.

That said, I don't think a local git Repo object should be used here at all. Ultimately, this object is being used to inspect the local git branch for its name. That branch name is then being used as the infrahub branch against which to execute a graphQL query. To my understanding, the local git repo's branch and the branch in infrahub that should be queried are completely separate entities (although I might be mistaken). If this is the case, It might be best to remove the Repo (.git) object all together and have branch_name default to an empty string (or the default branch in infrahub) unless declared?

Steps to Reproduce

  1. Create a file called main.py. In it, do the following:
from infrahub_sdk.transforms import InfrahubTransform

class TagsTransform(InfrahubTransform):
    query = "tags_query"

def main():
    transform = TagsTransform()
    print(transform.branch_name)

if __name__ == "__main__":
    main()
  1. Run the script and see that an attribute error is raised
python main.py

Traceback (most recent call last):
  File ".../main.py", line 8, in <module>
    main()
  File ".../main.py", line 5, in main
    print(transform.branch_name)
          ^^^^^^^^^^^^^^^^^^^^^
  File ".../infrahub-sdk-python/infrahub_sdk/transforms.py", line 82, in branch_name
    if not self.git:
           ^^^^^^^^
AttributeError: 'TagsTransform' object has no attribute 'git'

Additional Information

@ogenstad mentioned in comments on #7 that we should be initializing TransformInstance with an optional git parameter, then use .git as a getter/setter. This makes a lot of sense, though because I'm not sure we actually want a Repo() instance at all, I thought it might be best to create a separate issue to discuss and PR to resolve.

ogenstad commented 2 weeks ago

A question around this, was it a bug that you actually ran into or just observed by looking at the code (as it's incorrect as written).

Some background around this, the original idea was that you should be able to develop your external repository and run tests within a branch and if you're working in another branch on your local repo infrahubctl could pick up this information to target this branch instead of using the main branch (I'm not completely sure I like this concept).

I think we should fix this regardless but I'm not sure we run into it under normal circumstances if we always provide a branch.

As a note we also have the option to set the default branch from git using an environment variable (https://docs.infrahub.app/python-sdk/reference/config#default_branch_from_git). I think this environment variable should be enough (even though that in itself is problematic as it requires GitPython which in turn requires a git binary to be installed on the system.

I'd say the first step would be to correct this mistake by having a setter_getter or at the very least setting self.git to be Optional and default to None. After this I think the param should be phased out in favor of setting the variable for the SDK. Another issue there is that we should only import GitPython in the SDK if this variable is set as it should (if I don't remember this incorrectly) only be used if the INFRAHUB_DEFAULT_BRANCH_FROM_GIT is used. (we have an issue around this in #16)