ManageIQ / manageiq-automation_engine

Automation engine for ManageIQ
Apache License 2.0
11 stars 74 forks source link

[StateVarHash] Rely on created_at timestamp #479

Closed NickLaMuro closed 2 years ago

NickLaMuro commented 3 years ago

Depends on: https://github.com/ManageIQ/manageiq-schema/pull/605

Follow up to: https://github.com/ManageIQ/manageiq-automation_engine/pull/478 (merge after)

Now that timestamps have been added to BinaryBlob records:

https://github.com/ManageIQ/manageiq-schema/pull/605

Don't set resource_id to a value that is based on Time.now and rely on the newly created column

Links

coveralls commented 3 years ago

Pull Request Test Coverage Report for Build 5880


Totals Coverage Status
Change from base Build 5849: 0.007%
Covered Lines: 5055
Relevant Lines: 5895

💛 - Coveralls
Fryguy commented 2 years ago

@NickLaMuro I believe this PR is now basically a refactor, right? (though I see you still have 2 commits for backport purposes) So, this should be able to be merged independently?

NickLaMuro commented 2 years ago

@Fryguy Well, it is built on top of https://github.com/ManageIQ/manageiq-automation_engine/pull/478 so it is intended to be a follow up. The point of the first is that it could be backported by itself (no migration required), but then this one is the "proper fix".

Really, https://github.com/ManageIQ/manageiq-automation_engine/pull/478 should be merged then I rebase this to have just the one commit.

NickLaMuro commented 2 years ago

My branch was a bit outdated because I forgot about this PR for a bit, so I think that might be why the tests failed(?).

We'll see...

miq-bot commented 2 years ago

Checked commits https://github.com/NickLaMuro/manageiq-automation_engine/compare/cb7056d9f79936d8a9bc9c393863887adb3046a8~...1f7dadd08b98c702dce115e53e36c8ccbcdceb96 with ruby 2.6.3, rubocop 1.13.0, haml-lint 0.35.0, and yamllint 1 file checked, 0 offenses detected Everything looks fine. :cookie:

Fryguy commented 2 years ago

Well, it is built on top of #478 so it is intended to be a follow up.

Ahhh ok...that makes sense.

kbrock commented 2 years ago

part of https://github.com/ManageIQ/manageiq/pull/21731

This PR purges via a created_at. So it depends upon that column.

Fryguy commented 2 years ago

Skipping backport to najdorf, because it is already in the branch.