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.

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 #29093 - change from superficial copy to deep copy of fields #348

Merged
merged 1 commit into from
Aug 12, 2021

Conversation

adiabramovitch
Copy link
Contributor

No description provided.

Copy link
Contributor

@yifatmakias yifatmakias left a comment

Choose a reason for hiding this comment

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

Thanks @adiabramovitch ,
Notice that the hammer_cli_foreman tests are failing with this error:
image

@@ -11,7 +11,8 @@ def initialize(options = {})
@all = []
@children = []
@options = options
@creator = options[:creator] || Class.new(HammerCLI::Apipie::Command)
Copy link
Member

Choose a reason for hiding this comment

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

FYI: I'm currently working on a little bit different approach for option creation and this anonymous class will disappear :) So, 👍 for the solution, but it should wait a bit before we get rid of this class.

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, @adiabramovitch! Since this change doesn't break tests for plugins anymore we can finally merge it :)

@ofedoren ofedoren merged commit c8430df into theforeman:master Aug 12, 2021
@ekohl
Copy link
Member

ekohl commented Aug 16, 2021

Looks like this broke nightly again, just like in #345 (comment): https://ci.theforeman.org/job/foreman-nightly-rpm-pipeline/1160/tapResults/

not ok 6 import motd puppet class
# (in test file fb-test-puppet.bats, line 64)
#   `hammer proxy import-classes --name $(hostname -f)' failed with status 70
# Error: singleton can't be dumped

@ofedoren
Copy link
Member

Looks like this broke nightly again, just like in #345 (comment): https://ci.theforeman.org/job/foreman-nightly-rpm-pipeline/1160/tapResults/

@ekohl, yeap, I'm aware of that: #353

@ekohl
Copy link
Member

ekohl commented Aug 16, 2021

Thanks!

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.

5 participants