(IAC-1218) - docker_params_changed should be run by agent #705
Conversation
docker_params_changed is a functionBreaking changes to this file MAY impact these 1 modules (near match):docker_params_changed is a functionBreaking changes to this file MAY impact these 1 modules (near match):docker::run is a typeBreaking changes to this file WILL impact these 6 modules (exact match):Breaking changes to this file MAY impact these 21 modules (near match):
This module is declared in 6 of 576 indexed public
|
Codecov Report
@@ Coverage Diff @@
## main #705 +/- ##
==========================================
- Coverage 23.42% 21.44% -1.98%
==========================================
Files 20 20
Lines 760 830 +70
==========================================
Hits 178 178
- Misses 582 652 +70
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Excellent work - it's been a big ticket! 😃
From a preliminary look, it seems sound enough although I must admit it's a little bit hard to follow the detect_changes method as there are a lot of conditionals in here.
I suggested some potential refactor ideas to make it a bit more readable, but, just some more comments with example docker inspect output would help a bit more for context. It could also be useful to go over this in a Show & Tell session to step through what's happening. Unit tests may also be another way to achieve this, especially for folks not as familiar with docker and subsequently, what this method is trying to do.
| "rm -f #{cidfile}" | ||
| end | ||
| _stdout, _stderr, _status = Open3.capture3(delete_command) | ||
| # logger.puts("### BEGIN remove_cidfile\n\n") |
There was a problem hiding this comment.
If we're not outputting these logs anywhere, we should probably delete these lines. I assume they were there to assist in the development process?
There was a problem hiding this comment.
I'm planning to send the logs to the journalctl, but didn't work as I want to work(without pre-configuration) so I decide to comment them and change them sometime in the future.
| "powershell.exe -Command \"& {#{cmd}}\" " | ||
| end | ||
|
|
||
| def remove_cidfile(cidfile, osfamily, _logger = nil) |
There was a problem hiding this comment.
I see _logger being set to nil in the methods within this file, but I never see _logger being used or passed anywhere else where it is used?
There was a problem hiding this comment.
Also, I let the logger parameter to use in a 'debug' mode. If you want to see some logs from just need to uncomment the line with the specific log.
| # logger.puts("### END create_container\n\n") | ||
| end | ||
|
|
||
| def detect_changes(opts) # rubocop:disable Metrics/CyclomaticComplexity,Metrics/PerceivedComplexity |
There was a problem hiding this comment.
This method is quite big - it might be good to refactor some of the code out in to separate methods or potentially create an object to handle the docker inspect output and allow us to query what we want from it a bit easier. E.g.
class DockerInspectInspector
def initialize(opts, docker_inspect_output)
@opts = opts
@docker_inspect_output = JSON.parse(docker_inspect_output)[0]
@opt_volumes_count = @opts['volumes'].scan(%r{(?=:)}).count
@os_family_windows = opts['osfamily'] == 'windows'
@opts_volumes_valid = @opts['volumes'].is_a? String
end
def mounts_changed
return false unless @opts_volumes_valid
@opts['volumes'].include?(':') && @opts['volumes'] != @docker_inspect_output['Mounts'].to_a[0]
end
def volumes_changed
return false unless @opts_volumes_valid
!@opts['volumes'].include?(':') && @opts['volumes'] != @docker_inspect_output['Config']['Volumes'].to_a[0]
end
def volume_count_equal(i)
return false unless @opts_volumes_valid
@opt_volumes_count == i
end
def param_changed?
return true if mounts_changed && !@os_family_windows
return true if volumes_changed && !@os_family_windows
return true if volume_count_equal(2) && volumes_changed && !@os_family_windows
volume_count_equal(1) && volumes_changed && @os_family_windows
end
end...and then we can call the methods within the class to achieve what we want:
stdout, stderr, status = Open3.capture3("docker inspect #{opts['sanitised_title']}")
docker_inspect_inspector = DockerInspectInspector.new(opts, stdout)
param_changed = docker_inspect_inspector.params_changed?There was a problem hiding this comment.
That class is a very rough idea - do let me know if you want to go down that road before implementing. It is likely not the best way to do it, but hopefully it serves as a potential idea
There was a problem hiding this comment.
That's a great idea. I have thought to split into multiple functions, but using OOP seems to be a more readable solution.
I think this implementation will not remain in this state, it will be changed using resource API as @DavidS's recommendation.
I think all improvements will be implemented in a further ticket.
There are many improvements to be done: splitting this big function into small functions(using OOP), using ruby-pwsh,, using journalctl for logging and many other small improvements.
There was a problem hiding this comment.
Just a few minor comments, given there's a ticket to address some of the other issues at a later date.
I'm still (kind of) the opinion we shouldn't commit commented out code, as good practise. If you're going to make use of the logger to journalctl at a later stage, I would just make it part of that commit.
Co-authored-by: sanfrancrisko <[email protected]>
Co-authored-by: sanfrancrisko <[email protected]>
Co-authored-by: sanfrancrisko <[email protected]>
No description provided.