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

Fix log copy #252

Merged
merged 9 commits into from Jan 7, 2015
Merged

Fix log copy #252

merged 9 commits into from Jan 7, 2015

Conversation

jreidinger
Copy link
Member

No description provided.

require "installation/finish_client"

module Installation
class CopyLogsFinish < ::Installation::FinishClient
Copy link
Contributor

Choose a reason for hiding this comment

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

Just asking. Do we have any convention on naming all these clients living under lib/installation? Like always using WhateverFinish for clients inheriting from FinishClient?

Copy link
Member Author

Choose a reason for hiding this comment

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

actually no convention...in bootloader it is simple Bootloader/FinishClient but here it can create collisions so I name it this way. CopyLogs without finish can confuse reader as it can be seen as common class to copy logs from one place to another.

it "copies logs from instalation to target system" do
allow(Yast::WFM).to receive(:Read).and_return(["y2start.log"])

expect(Yast::WFM).to receive(:Execute).with(anything(), /cp/).at_least(:once)
Copy link
Contributor

Choose a reason for hiding this comment

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

The y2start.log returned by the previous call is not used in the expectation. Feels strange.

Copy link
Member Author

Choose a reason for hiding this comment

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

fair comment. I will add it.

@ancorgs
Copy link
Contributor

ancorgs commented Jan 7, 2015

LGTM.

BTW, if you start working on the SCR helpers, as suggested by you comment, I'm not unhappy with my last attempt. https://github.com/ancorgs/yast-security/blob/pre907907_cleanup/test/SCRStub.rb

@jreidinger
Copy link
Member Author

@ancorgs yep, I plan to reuse your stub, but to be generic enough, there is some problems like .target.bash and .target.bash_output and also .local.bash for WFM and not SCR. I worry it won't be extra nice and probably some parts will be repetitive

jreidinger added a commit that referenced this pull request Jan 7, 2015
@jreidinger jreidinger merged commit 971cae6 into master Jan 7, 2015
@jreidinger jreidinger deleted the fix_log_copy branch January 7, 2015 16:08
@ancorgs
Copy link
Contributor

ancorgs commented Jan 7, 2015

That's why I removed things like expect_to_execute in the last incarnation and just implemented a exec_bash matcher for ".target.bash". My idea was to have a generic exec matcher that checks again every possible form and them some specific matchers like exec_bash, exec_bash_output and so on.

@jreidinger
Copy link
Member Author

@ancorgs unfortunatelly this won't work as

def exec_bash(command)
receive(:Execute).with(path(".target.bash"), command)
end

work only for SCR, because WFM use .local.bash as System agent is attached to path .local and not .target. It is quite stupid and hard to handle. It also do not solve issue with .target.bash_output

@ancorgs
Copy link
Contributor

ancorgs commented Jan 7, 2015

@jreidinger ahm. I see.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants