Skip to content

hetznerctl: Add config parser for _cmd options#44

Draft
lucc wants to merge 6 commits intoaszlig:masterfrom
lucc:feature/password_cmd
Draft

hetznerctl: Add config parser for _cmd options#44
lucc wants to merge 6 commits intoaszlig:masterfrom
lucc:feature/password_cmd

Conversation

@lucc
Copy link

@lucc lucc commented Jul 24, 2021

This wraps the config parser class from the stdlib to transparently fall back to a *_cmd option if an option is not set. The *_cmd option is expected to be a string to be passed to the python subprocess module. The return value of the that is the option value that is returned.

open questions

  • What python versions should be supported? Currently the code requires py3.7
  • How should errors when executing a command be treated/reported? Currently an exception is thrown.
  • Should the _cmd option be interpreted by the shell or as a plain command? I would prefer shell.

Copy link
Owner

@aszlig aszlig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for the PR, only added a few nitpick comments. However what I'm not so sure whether inheriting from RawConfigParser is a good idea, since overriding methods could introduce conflicts with upstream. I'd rather prefer a dedicated function that returns something like a Dict[str, str] instead and which handles _cmd. This could also have some logic that returns an error if the configuration file contains foo and foo_cmd.

Addressing #43 (comment): So far I was trying to be as backwards-compatible as possible, supporting Python all the way down to Python 2.7.

The reason for this is that NixOps 2.0 is yet to be released and version 1.x only supports Python 2.x. However, I'm leaning towards making a version 1.x of this library (only supporting Python 3) and release fixes for the 0.x version in a different branch.

@aszlig
Copy link
Owner

aszlig commented Jul 24, 2021

To address the questions:

  • What python versions should be supported? Currently the code requires py3.7

Well, ideally the whole project would switch to using type hints, so I'd say at least Python 3.5.

  • How should errors when executing a command be treated/reported? Currently an exception is thrown.

Ideally, I'd lean towards Haskell's Either or Result in Rust, but since we're using Python here I'd say let's leave it at an exception until the whole thing is refactored.

  • Should the _cmd option be interpreted by the shell or as a plain command? I would prefer shell.

I'm not a fan of all those /bin/sh -c cmd shenanigans and would prefer execve directly instead, but I'd say most people would expect that shell syntax is allowed so I think that's fine.

@aszlig
Copy link
Owner

aszlig commented Jul 24, 2021

Btw. these are the Python versions that are tested right now: https://headcounter.org/hydra/jobset/aszlig/hetzner#tabs-jobs

Noted by the first review and by pycodestyle.
@lucc lucc force-pushed the feature/password_cmd branch from 3625df0 to 601d4b8 Compare July 25, 2021 19:11
lucc added 2 commits July 25, 2021 21:24
This also updates the usage of the subprocess module to support python
2.7.
@lucc lucc force-pushed the feature/password_cmd branch from 5f69acf to d80a823 Compare July 25, 2021 19:25
@lucc
Copy link
Author

lucc commented Jul 25, 2021

I tried to fix some things you noted. I still don't understand what kind of function you envision that returns a Dict[str, str]. Can you explain that?

@aszlig
Copy link
Owner

aszlig commented Nov 20, 2021

Sorry for the delay, I should probably try to find a way how to get notifications for my own repositories (I get notifications for like everything except the things that actually matter X-D)...

I tried to fix some things you noted. I still don't understand what kind of function you envision that returns a Dict[str, str]. Can you explain that?

Something like this:

def parse_config(file: Path) -> Dict[str, str]:
    # ... do all kinds of internal stuff with RawConfigFileParser and whatnot...
    return parsed_config

If I didn't miss anything, a plain old dictionary should be sufficient for representing the data.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants