-
Notifications
You must be signed in to change notification settings - Fork 15
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
ENH: Add operating system field to event-tracklog #440
Conversation
@@ -275,8 +297,8 @@ class TracklogEvent(BaseModel): | |||
examples=["created", "updated", "merged"], | |||
) | |||
user: User | |||
sysinfo: Optional[SystemInformation] = Field( | |||
default=None, | |||
sysinfo: SystemInformation = Field( |
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.
sysinfo will always be present and always contain the platform field.
Is How do we know that this works on all platforms? Tested so far on Mac, I assume. Those that comes to mind:
|
Are there security concerns to take into account? These data are to be shared with externals, we must assume that they will be made available to anyone. Are we OK exposing exactly which platforms we are running FMU on internally? |
Its a builtin python package, i would be amazed if the does not work on all python supported platforms. However, how can i test for other platforms / operating systems? |
Good question, if there is a known security weakness in a version we using this could potentially be used as a discriminator. |
There should be people in Equinor to ask about this... |
Yes agree it should work in general. For me, this is "nice to have" information; not something "critical" we need to make dataio/sumo work. |
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.
OK for me
Change since last review.
|
I checked out the source for plaform quick, and i see code for windows in there. So for now, i think its safe to assume so. |
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 think this is fine, but we should confirm the security implications before we merge.
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.
Based on inputs from community, I consider the potential risk very low. Still a question about how much is needed to include here, but that can also be adjusted over time.
Resolves: #387
Schema will be updated as soon as we decide to publish a new schema