Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fixes #35905 - Fix time to pickup #99

Merged
merged 1 commit into from
Jan 16, 2023
Merged

Fixes #35905 - Fix time to pickup #99

merged 1 commit into from
Jan 16, 2023

Conversation

adamruzicka
Copy link
Contributor

@adamruzicka adamruzicka commented Jan 6, 2023 •
edited

TODOs:

  • ygg up, time to pickup shorter than job, job should be successful
  • ygg down, job should fail due to time to pickup
  • ygg up, timeout to kill shorter than job, job should fail
  • ygg down, timeout to kill shorter than time to pickup, job should fail
  • ygg up, no timeouts, manual cancel
  • ygg down, no timeouts, manual cancel

@adamruzicka adamruzicka marked this pull request as ready for review January 6, 2023 15:32
ofedoren
ofedoren approved these changes Jan 16, 2023
View reviewed changes
Copy link
Member

@ofedoren ofedoren left a comment

Choose a reason for hiding this comment

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

Thanks, @adamruzicka, seems to solve the issue. One small noobie question, but otherwise LGTM.

lib/smart_proxy_remote_execution_ssh/api.rb
do_authorize_with_ssl_client

with_authorized_job(job_uuid) do |job_record|
{}
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need an empty endpoint?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This just just a workaround to yggdrasil doing things its own way. Upon receiving a message, yggdrasil needs to call an url, otherwise it just fails :/

Copy link
Member

Choose a reason for hiding this comment

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

In that case it's good to go :)

@ofedoren ofedoren merged commit faa2696 into theforeman:master Jan 16, 2023
@adamruzicka adamruzicka deleted the time-to-pickup branch January 16, 2023 17:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ofedoren ofedoren ofedoren approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@adamruzicka @ofedoren @theforeman-bot