Add nodename to response #27
Conversation
scrapyd_k8s/launcher/docker.py
Outdated
| @@ -21,7 +21,11 @@ def __init__(self, config): | |||
| self._docker = docker.from_env() | |||
|
|
|||
| def get_node_name(self): | |||
| return os.getenv('HOSTNAME') | |||
| hostname = os.getenv('HOSTNAME') | |||
| if hostname in [None, "", "null"]: | |||
There was a problem hiding this comment.
Most often HOSTNAME exists in linux based systems but it's not part of posix family standard so there can be cases when it's null, this is the source my thoughts are based on:
https://superuser.com/questions/132489/hostname-environment-variable-on-linux#comment1214603_132500
There was a problem hiding this comment.
What brings HOSTNAME that gethostname() doesn't bring? (Perhaps the ability to override the node name from the environment, but then I'd like it across launcher backends, and not provide that ability here.) So I would just use gethostname(). You can already override the node name in the config file.
There was a problem hiding this comment.
Yes, true, os.getenv is just a call to "third-party" program while gethostame is part of core if I understand correctly so better just use gethostname and in case it's null then use "default"?
There was a problem hiding this comment.
Well, both are from the core Python library, but os.getenv() looks in the environment, and gethostname() does a syscall to request the system hostname. The Superuser question shows that HOSTNAME may not be a proper environment variable (and if it is may depend on the Linux distribution or other *nix system flavour, in any case it is not part of the POSIX spec), so that you cannot really rely on it.
There was a problem hiding this comment.
Base on
https://pythontic.com/modules/socket/gethostname
and https://man7.org/linux/man-pages/man2/sethostname.2.html
I will delete if statement that checks if hostname value is None/null or empty
|
After a CI rerun also the Kubernetes test is fine again 😅 |
…le, rewrite function to retrieve deployment name and pod namespace, use socket.gethostname instead of sys popopen
scrapyd_k8s/launcher/docker.py
Outdated
| @@ -21,7 +21,11 @@ def __init__(self, config): | |||
| self._docker = docker.from_env() | |||
|
|
|||
| def get_node_name(self): | |||
| return os.getenv('HOSTNAME') | |||
| hostname = os.getenv('HOSTNAME') | |||
| if hostname in [None, "", "null"]: | |||
There was a problem hiding this comment.
What brings HOSTNAME that gethostname() doesn't bring? (Perhaps the ability to override the node name from the environment, but then I'd like it across launcher backends, and not provide that ability here.) So I would just use gethostname(). You can already override the node name in the config file.
There was a problem hiding this comment.
Thanks!
It's good to always review your own PR. Doing a review of all changes, shows me that there are still unrelated whitespace changes with meaning (e.g. the newlines in the tests do convey something about the grouping - e.g. a comment of a missing test does not belong to the following method, so you can't just remove the empty newline. Linters and autoformatters don't have the final word!)
Could you still review and change this? When the whitespace is correct, the PR can be merged.
scrapyd_k8s/launcher/docker.py
Outdated
| hostname = os.getenv('HOSTNAME') | ||
| if hostname in [None, "", "null"]: | ||
| hostname = socket.gethostname() | ||
| hostname = socket.gethostname() | ||
| return hostname |
There was a problem hiding this comment.
Ah, also just return socket.gethostname() here, for brevity.
I deleted whitespace added by code formatter, if you notice there is still some whitespace that you think should be different - let me know. I am now having my Mac wiped out so I won't be able to access it for some time. |
No description provided.