fix: Handle file-path in content #17
Conversation
| def check_is_not_path(cls, ssh_key_content: str | None) -> str | None: | ||
| if ssh_key_content is None: | ||
| return None | ||
| if os.path.exists(ssh_key_content): |
There was a problem hiding this comment.
Could try / except pathlib conversion to path but this doesn't raise exceptions and directly checks whether the value points to a file. (Could point to folder conceptually 🤔 )
There was a problem hiding this comment.
I think that I'd rather raise a more understandable error here instead of just magically solving the issue. Reason being that we're inheriting the env variables from the main SDK so wouldn't want to mess with that IMO.
So perhaps just
| if os.path.exists(ssh_key_content): | |
| if os.path.exists(ssh_key_content): | |
| raise ValueError("It looks like your environment variable `ENCORD_SSH_KEY` is a file path and not the private ssh key content. Did you mean to set `ENCORD_SSH_KEY_PATH` instead?") |
There was a problem hiding this comment.
I disagree. I don't see why we wouldn't just magical resolve the issue? The point is not being right, it's making it work appropriately. If customers have their env keys in one place, then they may rely on that behaviour.
| def check_is_not_path(cls, ssh_key_content: str | None) -> str | None: | ||
| if ssh_key_content is None: | ||
| return None | ||
| if os.path.exists(ssh_key_content): |
There was a problem hiding this comment.
I think that I'd rather raise a more understandable error here instead of just magically solving the issue. Reason being that we're inheriting the env variables from the main SDK so wouldn't want to mess with that IMO.
So perhaps just
| if os.path.exists(ssh_key_content): | |
| if os.path.exists(ssh_key_content): | |
| raise ValueError("It looks like your environment variable `ENCORD_SSH_KEY` is a file path and not the private ssh key content. Did you mean to set `ENCORD_SSH_KEY_PATH` instead?") |
Failed for me and for customer FYLD.