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

Batch disconnect method for ContainerImage #15698

Merged

Conversation

Ladas
Copy link
Contributor

@Ladas Ladas commented Aug 1, 2017

Batch disconnect method for ContainerImage.

Batch disconnect method for ContainerImage
@Ladas
Copy link
Contributor Author

Ladas commented Aug 1, 2017

@miq-bot assign @agrare
@miq-bot add_label enhancement, performance
cc @cben

Copy link
Contributor

@cben cben left a comment

Choose a reason for hiding this comment

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

[orthogonal to this PR: @enoodle @zakiva why do we disconnect images from registries? I'd expect us to keep it, and archive registries too.]

@@ -97,5 +97,10 @@ def disconnect_inv
save
end

def self.disconnect_inv(ids)
_log.info "Disconnecting ContainerImages ids [#{ids}]"
Copy link
Member

Choose a reason for hiding this comment

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

Can you more closely match the log message from the normal disconnect_inv ?
Maybe "Disconnecting Images [#{ids}] from EMS [#{ext_management_system.name}] id [#{ext_management_system.id}]"

Copy link
Contributor Author

@Ladas Ladas Aug 2, 2017

Choose a reason for hiding this comment

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

right, I don't have the EMS object here. Not sure if I should pass it for the purpose of logging? the EMS can be found in the log by looking at the process id though.

Copy link
Member

Choose a reason for hiding this comment

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

Ah right, just "Disconnecting Images... then

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Reword the log message based on review
@miq-bot
Copy link
Member

miq-bot commented Aug 2, 2017

Checked commits Ladas/manageiq@2c10dbf~...272744c with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
1 file checked, 2 offenses detected

app/models/container_image.rb

Copy link
Member

@agrare agrare left a comment

Choose a reason for hiding this comment

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

👍

@agrare agrare merged commit 6a4d3ed into ManageIQ:master Aug 2, 2017
@agrare agrare added this to the Sprint 66 Ending Aug 7, 2017 milestone Aug 2, 2017
@cben
Copy link
Contributor

cben commented Sep 12, 2017

Fun fact: the singular #disconnect_inv methods load ExtManagementSystem for the sole purpose of logging what's disconnected from which EMS.
I'm not sure that happens for every disconnected record in prod, or is somehow cached, but if yes, half the win of batched disconnect is batched logging :-).

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