[GitRepository] Broadcast delete of repo_dir #19123
Conversation
|
@miq-bot add_label ivanchuk/yes |
|
I'm confused...I made a couple of comments on mobile and they are not here 😭 |
NickLaMuro
force-pushed
the
git_repository_broadcast_filesystem_deletes
branch
from
48a1220
to
3c86f62
Compare
NickLaMuro
force-pushed
the
git_repository_broadcast_filesystem_deletes
branch
from
3c86f62
to
b148e9d
Compare
NickLaMuro
force-pushed
the
git_repository_broadcast_filesystem_deletes
branch
from
b148e9d
to
568a805
Compare
|
sync'd with master |
When a GitRepository record is deleted, broadcast the action of deleting the filesystem info to all of the known active servers. Since `EmbeddedAnsible` relates to a `GitRepository` record in it's `ConfigurationScriptSource` for it's local git file system management, the specs for it also needed to be updated so that they include running the job that was pushed on to the queue (instead of it being deleted right after a destroy from a callback)
NickLaMuro
force-pushed
the
git_repository_broadcast_filesystem_deletes
branch
from
568a805
to
db5b3c5
Compare
|
Checked commit NickLaMuro@db5b3c5 with ruby 2.4.6, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0 |
| @@ -383,6 +383,9 @@ def playbooks_for(repo) | |||
| expect(Notification).to receive(:create!).with(notification_args('deletion', {})) | |||
| record.delete_in_provider | |||
|
|
|||
| # Run most recent queue item (`GitRepository#broadcast_repo_dir_delete`) | |||
| MiqQueue.get.deliver | |||
There was a problem hiding this comment.
I realize this is probably going to trigger @carbonin 's "this isn't a unit test" argument, but I figured removing the:
expect(git_repo_dir).to_not existWas a worse compromise. Mostly since while technically this is already mostly testing a lot of "Rails inner workings" when testing this method, we still want to confirm the business logic of it all is hooked up correctly, and that does require going outside of the scope of this method a bit.
However, I don't think relying on the tests from above for this is the answer either, since if we ever decide to use something besides GitRepository or MiqQueue, this spec would fail when we want it to and inform us that something in this model needs to change. I feel like that is some "brittleness" that we can afford to confirm that this is still doing it's job when things are inevitably refactored, and save us from having other groups inform of us this first.
There was a problem hiding this comment.
Also, this is what was causing the specs to fail and cause this PR to be red. Hence the change in the first place.
There was a problem hiding this comment.
It's no worse than how I feel about the rest of the MiqQueue#deliver calls in the other specs in this PR so I'd rather just get it in before I head out for the day 😉
There was a problem hiding this comment.
Was also partially bringing this up since there was a lengthy discussion about this (and similar issues) by the two of us Friday, and I wanted this to be mentioned somewhere public instead of it just being something simply discussed out of band.
…lesystem_deletes [GitRepository] Broadcast delete of repo_dir (cherry picked from commit 89164c4)
|
Ivanchuk backport details: |
Built on top of #19122
Updates
GitRepository#destroyto sendFileUtils.rm_rfcalls to the queue so it can be performed on all servers. The jobs should be idempotent and able to work even after the record is removed from the database.Links