-
Notifications
You must be signed in to change notification settings - Fork 290
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
Fix dictionary update in try_push_job_info() #1673
Conversation
ab1eb81
to
6613646
Compare
@zmc Just want to confirm with you that this makes sense since you initially wrote this part a long time ago and there weren't really any issues until I tested out my docker-compose script. |
@kamoltat Conceptually you're definitely right here; Out of curiosity, what problem did this cause with the compose deployment? |
@zmc it is not used in the current master of technology but with #1650, it is used in https://github.com/ceph/teuthology/pull/1650/files#diff-518b3a856fe844adcc19187dade87bd07738e36d20a528eef4834be57b0ba4d0R116 and without the fix, the dispatcher in #1650 faced an issue with trying to create and deleting archive files. |
@kamoltat that makes sense, thanks! Would you mind rebasing and repushing just to get the unit tests to run? |
6613646
to
76af0ba
Compare
the function `try_push_job_info()` is not updating `job_info` dictionary properly since we want to update `job_info` with `extra_info`, however, in lines 498 and 499 we are assigning `job_info` to a copy of `extra_info` and updating `job_info` with `job_config` which is incorrect. Instead, we should assign `job_info` with a copy of `job_config` and update `job_info` with `extra_info`
76af0ba
to
11ef3dd
Compare
the function
try_push_job_info()
is notupdating
job_info
dictionary properly sincewe want to update
job_info
withextra_info
,however, in lines 498 and 499 we are assigning
job_info
to a copy ofextra_info
and updatingjob_info
withjob_config
which is incorrect.Instead, we should assign
job_info
witha copy of
job_config
and updatejob_info
withextra_info