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

[Scanner] Extend execution scanner framework with an entity type param #3402

Merged
merged 5 commits into from
Jul 23, 2020

Conversation

mkolodezny
Copy link
Contributor

@mkolodezny mkolodezny commented Jul 21, 2020

What changed?

This is the first step to have a separate scanner workflow for current executions (and for other types in the future).
The next steps are:

  • introduce current execution type APIs
  • implement concrete execution check
  • register the scanner workflow configured for current executions

How did you test it?

unit tests

@coveralls
Copy link

coveralls commented Jul 21, 2020

Coverage Status

Coverage increased (+0.4%) to 67.677% when pulling d543790 on mkolodezny:scanner into 8da382d on uber:master.

@mkolodezny mkolodezny changed the title Extend execution scanner workflow to handle multiple entity types Extend execution scanner framework with an entity type param Jul 21, 2020
common/reconciliation/common/interfaces.go Show resolved Hide resolved
common/reconciliation/common/types.go Show resolved Hide resolved
common/reconciliation/common/types.go Show resolved Hide resolved
common/reconciliation/common/util.go Outdated Show resolved Hide resolved
common/reconciliation/common/util.go Show resolved Hide resolved
@mkolodezny mkolodezny marked this pull request as ready for review July 22, 2020 17:34
@mkolodezny mkolodezny requested a review from vancexu July 22, 2020 17:34
@@ -83,16 +83,21 @@ const (

// The following are types related to Invariant.
type (
// Execution is an execution which should be checked or fixed.
// Execution is a base type for executions which should be checked or fixed.
Execution struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

Another idea is turn Execution into an interface with Getter method for each field and let concrete and current records extend this interface to include more methods. Then we can be more specific when writing function signatures and don't need to use interface{} everywhere. I think this can also address @andrewjdawson2016 's comments

This method should take in common.ConcreteExectuion. Invariants for concrete execution cannot be applied on current execution (and the reverse is also true) so at this level in code I don't think it makes sense to still be dealing with interfaces (or even dealing with union types at this level if we go with that path).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I like that proposal cc @meiliang86 Lets try to do it in the next change for step "introduce current execution type APIs".

@mkolodezny mkolodezny merged commit 293d5c2 into cadence-workflow:master Jul 23, 2020
@mkolodezny mkolodezny deleted the scanner branch July 27, 2020 17:43
@mkolodezny mkolodezny changed the title Extend execution scanner framework with an entity type param [Scanner] Extend execution scanner framework with an entity type param Aug 1, 2020
yux0 pushed a commit to yux0/cadence that referenced this pull request May 4, 2021
…-workflow#3402)

* Extend execution scanner workflow to handle multiple entity types
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants