Skip to content

Fixes #37320 - Set IDs on outputs#88

Merged
adamlazik1 merged 4 commits into
theforeman:masterfrom
adamruzicka:output-ids
Dec 19, 2025
Merged

Fixes #37320 - Set IDs on outputs#88
adamlazik1 merged 4 commits into
theforeman:masterfrom
adamruzicka:output-ids

Conversation

@adamruzicka

@adamruzicka adamruzicka commented Apr 5, 2024

Copy link
Copy Markdown
Contributor

Requires:

TODO:

  • bump dependency on sp-dynflow

@adamruzicka

adamruzicka commented Nov 5, 2024

Copy link
Copy Markdown
Contributor Author

This should be ready once smart_proxy_dynflow-0.9.4 goes out.

@adamruzicka adamruzicka force-pushed the output-ids branch 2 times, most recently from f460185 to 967c69e Compare May 7, 2025 09:23
@adamruzicka adamruzicka force-pushed the output-ids branch 2 times, most recently from 26ffbe2 to 91a1f4c Compare June 25, 2025 10:20

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull Request Overview

This PR aims to resolve issue #37320 by setting IDs on outputs. Key changes include:

  • Bumping the dependency on sp-dynflow in the gemspec.
  • Propagating id and timestamp parameters to output publishing calls in the ansible runner.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
smart_proxy_ansible.gemspec Updated the runtime dependency constraint for smart_proxy_dynflow to require a minimum version.
lib/smart_proxy_ansible/runner/ansible_runner.rb Added id and timestamp parameters in methods handling host events and broadcast data.
Comments suppressed due to low confidence (1)

lib/smart_proxy_ansible/runner/ansible_runner.rb:132

  • [nitpick] Consider adding unit tests to verify that the 'id' and 'timestamp' parameters are correctly propagated through the publish_data_for and broadcast_data methods.
        publish_data_for(hostname, event['stdout'] + "\n", 'stdout', id: event['uuid'], timestamp: event['created']) if event['stdout']

Comment thread smart_proxy_ansible.gemspec Outdated

@adamlazik1 adamlazik1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

full ISO timestamp is converted to 2025.0 by smart_proxy_dynflow calling timestamp.to_f. Is that expected?

@adamlazik1 adamlazik1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM other than the comment above. But that would be solved in a different repo anyway.

@adamruzicka

Copy link
Copy Markdown
Contributor Author

Is that expected?

It's been a while since I put this together, but no, that doesn't sound right.

But that would be solved in a different repo anyway.

Eh, not really. It expects a time instance, not a string.

@adamruzicka

Copy link
Copy Markdown
Contributor Author

@adamlazik1 changed, could you please look once more?

@adamlazik1 adamlazik1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Right now I am getting "timestamp"=>1766156561.3115878 on the foreman side, which would be equivalent to Time.now.to_f, so it looks like it works as intended.

Also, I don't know how much this matters, probably not a lot, but should warning be thrown if parse_timestamp intercepts an exception instead of passing a new timestamp based on Time.now?

@adamruzicka

Copy link
Copy Markdown
Contributor Author

Also, I don't know how much this matters, probably not a lot, but should warning be thrown if parse_timestamp intercepts an exception instead of passing a new timestamp based on Time.now?

Probably not a whole lot, but added anyway

@adamlazik1 adamlazik1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@adamlazik1 adamlazik1 merged commit 33caff6 into theforeman:master Dec 19, 2025
9 of 10 checks passed
@adamlazik1

Copy link
Copy Markdown
Contributor

Thanks @adamruzicka!

@adamruzicka adamruzicka deleted the output-ids branch December 19, 2025 15:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants