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.

Sign up for GitHub

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

Jump to bottom

Truncate name of refresh task to 255 #16444

Merged
merged 2 commits into from
Nov 13, 2017
Merged

Truncate name of refresh task to 255 #16444

merged 2 commits into from
Nov 13, 2017

Conversation

yrudman
Copy link
Contributor

@yrudman yrudman commented Nov 10, 2017

After migrating older DB datatype t.string would have maximum length of 255.
See ManageIQ/manageiq-schema#125 for more details.

https://bugzilla.redhat.com/show_bug.cgi?id=1510605

This PR truncate length of string to 255 when attempting to create record in miq_tasks

@miq-bot add-label bug
@miq-bot assign @agrare

\cc @Fryguy

@yrudman yrudman mentioned this pull request Nov 10, 2017
Merged
@miq-bot miq-bot added the bug label Nov 10, 2017
agrare
agrare reviewed Nov 10, 2017
View reviewed changes
spec/models/ems_refresh_spec.rb Outdated
@@ -280,4 +280,16 @@ def assert_queue_item(expected_targets)
expect(MiqQueue.count).to eq(1)
end
end

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 remove the whitespace here?

@yrudman yrudman force-pushed the name-of-refresh-task-should-fit-255 branch from 8b4b2c1 to 53a3d1a Compare November 10, 2017 17:45
agrare
agrare reviewed Nov 10, 2017
View reviewed changes
spec/models/ems_refresh_spec.rb Outdated

describe ".create_refresh_task" do
it "create refresh task and trancates task name to 255 symbols" do
ems = double("Ems")
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 create an ems with a factory? e.g. ems = FactoryGirl.create(:ems_vmware, :name => "Ems")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

create_refresh_task called fromqueue_merge, I will move test under queue_merger context and will use already created instances

agrare
agrare reviewed Nov 10, 2017
View reviewed changes
spec/models/ems_refresh_spec.rb Outdated
ems = double("Ems")
ems_name = "SomeTestEmsName"
allow(ems).to receive(:name).and_return(ems_name)
described_class.send(:create_refresh_task, ems, "a" * 1000)
Copy link
Member

Choose a reason for hiding this comment

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

Same with the targets, how about:

vm  = FactoryGirl.create(:vm_vmware)
targets = 500.times.map { vm }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will do the same as with ems

agrare
agrare reviewed Nov 10, 2017
View reviewed changes
spec/models/ems_refresh_spec.rb Outdated
ems_name = "SomeTestEmsName"
allow(ems).to receive(:name).and_return(ems_name)
described_class.send(:create_refresh_task, ems, "a" * 1000)
task_name = MiqTask.last.name
Copy link
Member

Choose a reason for hiding this comment

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

EmsRefresh.create_refresh_task return the task so you don't need to find it, just do task = described_class.send(:create_refresh_task...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

@yrudman yrudman force-pushed the name-of-refresh-task-should-fit-255 branch 2 times, most recently from 5be442d to af182ea Compare November 10, 2017 19:19
@miq-bot
Copy link
Member

miq-bot commented Nov 10, 2017

Checked commits yrudman/manageiq@9333bef~...af182ea with ruby 2.3.3, rubocop 0.47.1, and haml-lint 0.20.0
2 files checked, 0 offenses detected
Everything looks fine. ⭐

agrare
agrare requested changes Nov 12, 2017
View reviewed changes
spec/models/ems_refresh_spec.rb Outdated
@@ -279,5 +279,14 @@ def assert_queue_item(expected_targets)
EmsRefresh.queue_merge([vm], ems)
expect(MiqQueue.count).to eq(1)
end

describe ".create_refresh_task" do
Copy link
Member

Choose a reason for hiding this comment

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

There is a queue_refresh_task context which calls this method, I think that context makes more sense for this test. No need to combine this into the queue_merge context just because it has ems and vm already defined.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

right, naming of those 2 methods sounds like this test should be under queue_refresh_task context, but .create_refresh_task was not invoked from .queue_refresh_taske, it called only once from .queue_merge https://github.com/yrudman/manageiq/blob/af182ea3587b445879185490efee03fafc3744ec/app/models/ems_refresh.rb#L181.
This is the reasoin why it was put under .queue_merge context,
I thought it would be confusing to put test for method under context where it was not invoked.

Copy link
Member

Choose a reason for hiding this comment

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

Hm we still call queue_merge from queue_refresh_task though so it should be fine there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, will move new test under queue_refresh_task context.

@yrudman
Copy link
Contributor Author

yrudman commented Nov 13, 2017

@miq_bot add-label gaprindashvili/yes, fine/yes

@yrudman yrudman force-pushed the name-of-refresh-task-should-fit-255 branch from af182ea to 37f634d Compare November 13, 2017 13:48
@agrare agrare merged commit f74530d into ManageIQ:master Nov 13, 2017
@agrare agrare added this to the Sprint 73 Ending Nov 13, 2017 milestone Nov 13, 2017
@yrudman yrudman deleted the name-of-refresh-task-should-fit-255 branch November 13, 2017 14:53
simaishi pushed a commit that referenced this pull request Nov 14, 2017
@agrare @simaishi
Merge pull request #16444 from yrudman/name-of-refresh-task-should-fi…
c6a82d3 
…t-255

Truncate name of refresh task to 255
(cherry picked from commit f74530d)

https://bugzilla.redhat.com/show_bug.cgi?id=1513123
@simaishi
Copy link
Contributor

Gaprindashvili backport details:

$ git log -1
commit c6a82d3b0da835fbec50f9d522a40bd836ea8fcc
Author: Adam Grare <agrare@redhat.com>
Date:   Mon Nov 13 09:32:25 2017 -0500

    Merge pull request #16444 from yrudman/name-of-refresh-task-should-fit-255
    
    Truncate name of refresh task to 255
    (cherry picked from commit f74530dd5716da2b8b36701f6e9adfe850b00134)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1513123

simaishi pushed a commit that referenced this pull request Nov 20, 2017
@agrare @simaishi
Merge pull request #16444 from yrudman/name-of-refresh-task-should-fi…
37dd982 
…t-255

Truncate name of refresh task to 255
(cherry picked from commit f74530d)

https://bugzilla.redhat.com/show_bug.cgi?id=1513124
@simaishi
Copy link
Contributor

Fine backport details:

$ git log -1
commit 37dd9820636c219df08dd50fcf89d5ebdca3c527
Author: Adam Grare <agrare@redhat.com>
Date:   Mon Nov 13 09:32:25 2017 -0500

    Merge pull request #16444 from yrudman/name-of-refresh-task-should-fit-255
    
    Truncate name of refresh task to 255
    (cherry picked from commit f74530dd5716da2b8b36701f6e9adfe850b00134)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1513124

@simaishi simaishi removed the fine/yes label Nov 20, 2017
d-m-u pushed a commit to d-m-u/manageiq that referenced this pull request Jun 6, 2018
@agrare @d-m-u
Merge pull request ManageIQ#16444 from yrudman/name-of-refresh-task-s…
1358810 
…hould-fit-255

Truncate name of refresh task to 255
(cherry picked from commit f74530d)

https://bugzilla.redhat.com/show_bug.cgi?id=1513124
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@agrare agrare agrare approved these changes

Assignees

@agrare agrare

Labels
bug fine/backported gaprindashvili/backported providers/inventory
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@yrudman @miq-bot @simaishi @agrare