-
Notifications
You must be signed in to change notification settings - Fork 100
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Imp/jtt 113 update env attributes on run time #304
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Ibrahim - I wonder how this
f"SECC Config is updated key = {key}: old value = " | ||
f"{self.config.as_dict()[key]} - new value = {value}" | ||
) | ||
self.config.update(new_config) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be better to call update() before logging the "updated" message?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
iso15118/shared/security.py
Outdated
|
||
def __get__(self, instance, owner): | ||
return os.path.join( | ||
shared_settings[SettingKey.PKI_PATH], "iso15118_2/certs/", super().value |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't self.value work here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it worked. updated
iso15118/shared/security.py
Outdated
return os.path.join( | ||
shared_settings[SettingKey.PKI_PATH], | ||
"iso15118_2/private_keys/", | ||
super().value, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as above, would self.value be better?
iso15118/shared/security.py
Outdated
shared_settings[SettingKey.PKI_PATH], | ||
"iso15118_2/private_keys/", | ||
super().value, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above regarding super().value
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it worked. updated
iso15118/shared/settings.py
Outdated
PKI_PATH = "pki_path" | ||
MESSAGE_LOG_JSON = "message_log_json" | ||
MESSAGE_LOG_EXI = "message_log_exi" | ||
V20_SERVICE_CONFIG = "v20_service_config" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the constants are in upper case, then logging becomes consistent with what we have today. When I ran secc the env were logged like this:
DEBUG 2023-10-03 21:47:41,980 - asyncio (54): Using selector: KqueueSelector
INFO 2023-10-03 21:47:41,981 - iso15118.secc.secc_settings (107): SECC settings:
INFO 2023-10-03 21:47:41,981 - iso15118.secc.secc_settings (109): pki_path : /Users/shalinnijel/Desktop/Switch/repos/josev_pro_repos/josev_pro_release/iso15118_service/iso15118_service/pki/
INFO 2023-10-03 21:47:41,981 - iso15118.secc.secc_settings (109): message_log_json : True
INFO 2023-10-03 21:47:41,981 - iso15118.secc.secc_settings (109): message_log_exi : False
INFO 2023-10-03 21:47:41,981 - iso15118.secc.secc_settings (109): v20_service_config : /Users/shalinnijel/Desktop/Switch/repos/iso15118_repos/iso15118_release_test/iso15118/shared/examples/secc/15118_20/service_config.json
INFO 2023-10-03 21:47:41,981 - iso15118.secc.secc_settings (109): enable_tls_1_3 : False
INFO 2023-10-03 21:47:41,982 - iso15118.secc.secc_settings (111): NETWORK_INTERFACE : en0
INFO 2023-10-03 21:47:41,982 - iso15118.secc.secc_settings (111): LOG_LEVEL : DEBUG
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see that in secc_settings, while logging, the env variables are automatically picked up - but here, we could still define the constants in upper case and that would fix the logging, right? Anything else that might cause issues if we make them lowercase?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
85f501d
to
81c4aee
Compare
def update_config(self, new_config: dict): | ||
self.config.update(new_config) | ||
|
||
def get_config(self) -> Config: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If both get_config() and update_config() are not required, then we could remove them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
iso15118/secc/secc_settings.py
Outdated
@@ -16,7 +16,7 @@ | |||
@dataclass | |||
class Config: | |||
iface: Optional[str] = None | |||
log_level: Optional[int] = None | |||
log_level: Union[str, int] = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed, this could be changed to Optional[str]
will allow us to update secs settings on run time.