-
Notifications
You must be signed in to change notification settings - Fork 95
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
Fixes #37321 - Rely on output id when inserting events #886
base: master
Are you sure you want to change the base?
Fixes #37321 - Rely on output id when inserting events #886
Conversation
b447333
to
eddb83c
Compare
template_invocation_id: template_invocation.id, | ||
event: data['exit_status'], | ||
timestamp: last[:timestamp], | ||
timestamp: data['exit_status_timestamp'] || last[:timestamp] + 1, |
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.
Do I read this right?
timestamp
will be either:
String
:data['exit_status_timestamp']
, which contains aFloat
converted to aString
, e.g.Time.now.utc.to_f
(https://github.com/theforeman/smart_proxy_dynflow/pull/136/files#diff-f3c605b1717c7b0b34ac0715dcb3867c32689fad5a09c7f73a9de47708fa326bR62)Time
:Time.zone.now
(https://github.com/theforeman/foreman_remote_execution/pull/886/files#diff-60c1608dbb7d26b97a00fa9c6508054d8dfde370bd3c99ed9e8fd4ff5647f7f9R36)
Also, in https://github.com/theforeman/smart_proxy_dynflow/pull/136/files#diff-606cc2cb76c4da10aa50e3b789b003c3452f553e0c425121cacca9a8af776e25R74 it's Time.now.utc
, but here it's Time.zone.now
. Shouldn't it be forced to use the same timezone?
Then, in https://github.com/theforeman/foreman_remote_execution/pull/886/files#diff-720316f4bf1726b14fa1324b7fecce4879e350f67177136c17cbf61e26aeba54R162 there is order
ing by timestamp
. But what's the value there? Does it compare numbers or strings? How does it work with inconsistencies above?
P.S. I might be totally lost, so please, take this as rather noob questions.
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.
String: data['exit_status_timestamp']
Eh, that somehow slipped through, we're supposed to parse this, same as we do on L31.
it's Time.now.utc, but here it's Time.zone.now. Shouldn't it be forced to use the same timezone?
sp-dynflow operates completely with utc timestamps as it makes things a lot easier. The utc timestamp gets dumped to some intermediate format, goes over the network and gets picked up here. On L31 we load it as utc timestamp and convert it to a timestamp in current user's timezone. This then gets stored as a utc timestamp in the database.
there is ordering by timestamp. But what's the value there? Does it compare numbers or strings? How does it work with inconsistencies above?
No, all the things in the db should be utc timestamps. Rails, as usual, does a lot of magic to make things happen implicitly.
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.
Fixed loading of the exit status timestamp, hopefully it should make more sense now.
Soft requires: