Refactor commands and add testing #72
Conversation
eguzki
commented
Nov 7, 2018
•
eguzki
commented
- Refactor copy service command
- Refactor update service command
- Add unit tests
| remote: remote(source)) | ||
| copy_service = create_new_service(source_service.show_service, destination, system_name) | ||
| context = create_context(source_service, copy_service) | ||
| tasks = [Tasks::CopyServiceProxyTask.new(context), |
There was a problem hiding this comment.
Just a matter of style, but I think this is better for several reasons:
tasks = [
...,
...,
]There was a problem hiding this comment.
Also, to enforce a uniform interface between them you can do:
COPY_SERVICE = [
Tasks::CopyServiceProxyTask,
Tasks::CopyMethodsTas,
...,
].freezeCOPY_SERVICE.map{ |task| task.new(context) }.each(&:call)There was a problem hiding this comment.
My first try was to pass context to the call method.
tasks.each { |task| task.call(context) }But later, I found convenient to have different context for each task. It turns out, so far, only one task needs extra param besides source/target service clients. I think this the most flexible way.
| class UpdateServiceSettingsTask | ||
| attr_reader :source_service, :copy_service, :target_system_name | ||
|
|
||
| def initialize(source_service:, copy_service:, target_name:) |
There was a problem hiding this comment.
When naming one source then the other should be target no? original/copy vs source/target.
| copy_methods = copy_service.methods | ||
| puts "original service hits metric #{source_service.hits['id']} has #{source_methods.size} methods" | ||
| puts "copied service hits metric #{copy_service.hits['id']} has #{copy_methods.size} methods" | ||
| m_m = ThreeScaleToolbox::Helper.array_difference(source_methods, copy_methods) do |method, copy| |
There was a problem hiding this comment.
Writing missing_methods is not that big a deal.
|
I am afraid, this PR is getting pretty big. Unfortunately, the refactor changes everything upside down into many pieces and each piece has its own unit test. Not sure to include integration tests in this PR. Will wok on them and can be included in this PR or later in another PR |
|
Import CSV command has not been refactored. No sure if it is useful and if anybody uses it. |
eguzki
changed the title
|
Opened another branch for integration tests. This PR is ready for review @mikz |
