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

DPLT-980 Tests for wildcard matching #86

Merged
merged 34 commits into from
Jun 28, 2023
Merged

Conversation

gabehamilton
Copy link
Collaborator

@gabehamilton gabehamilton commented May 31, 2023

This tests the local matching. Future changes and tests for wildcard matching with historical indexed data need #81
Block server option allows CamelCase (default) or snake_case data to be returned.

Copy link
Collaborator

@morgsmccauley morgsmccauley left a comment

Choose a reason for hiding this comment

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

Can you please add the jira issue key to the PR title

@@ -0,0 +1,34 @@
// fetch test blocks from block server
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we just add a shell script in indexer_rules_engine rather than adding an entire new project?

The same thing can be achieved with a simple curl command:

curl -o ./blocks/92476362.json https://70jshyr5cb.execute-api.eu-central-1.amazonaws.com/block/92476362`

And if we want to make it generic:

#!/bin/bash

mkdir -p ./blocks

# Iterate over all script arguments
for block_id in "$@"
do
  curl -o "./blocks/${block_id}.json" "https://70jshyr5cb.execute-api.eu-central-1.amazonaws.com/block/${block_id}"
done

Which could then be used like so:

./download_blocks 92476362 92476363

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Shell script is definitely simpler, which is a good goal. I try to write utilities like this in one of our programming languages since they tend to grow and then become something that needs to be called from an admin console, then by privileged users. Rust would make more sense here but I was trying to move fast.
I could put it in block_server, does that sound better?

Copy link
Collaborator

@morgsmccauley morgsmccauley Jun 5, 2023

Choose a reason for hiding this comment

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

Personally, I still would prefer shell script because:

  1. It doesn't require bootstrapping and maintaining another project
  2. It can live close to where it's used

Placing it in block_server puts it even further from it's use 😅

I think it would be trivial to spin this out in to another project when the requirement comes, I'm not a fan of optimising for requirements which aren't fully formed yet.

@gabehamilton gabehamilton changed the title Tests for wildcard matching DPLT-980 Tests for wildcard matching Jun 1, 2023
Copy link
Contributor

@roshaans roshaans left a comment

Choose a reason for hiding this comment

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

@gabehamilton A few questions:

For wildcard matching would *.near , *.account.near work?

Why is it that we want wildcards in the middle of an account name such as app.nea*crowd.near to work?

Also what about just * ? What is the expected behavior here?

@gabehamilton
Copy link
Collaborator Author

I think we should go with just wildcards for sub accounts to start in the UI. We can allow more later as people ask for more and as we test the historical processing with wildcards.
So *.nearcrowd.near, etc

Copy link
Collaborator

@morgsmccauley morgsmccauley left a comment

Choose a reason for hiding this comment

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

Couple small comments. Should we update the github actions to also run cargo tests? :D

@@ -115,3 +115,88 @@ fn build_indexer_rule_match_payload(
}
}
}

#[cfg(test)]
mod tests {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we also test for a wildcard which doesn't match anything to avoid false positives here?

@@ -2,3 +2,4 @@
.env*
redis/
*.log
/indexer/blocks/
Copy link
Collaborator

Choose a reason for hiding this comment

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

The tests explicitly depend on this data, should we just commit it to the repo?

@gabehamilton gabehamilton merged commit 46b2abf into main Jun 28, 2023
@gabehamilton gabehamilton deleted the DPLT-980_wildcard_tests branch June 28, 2023 21:45
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.

3 participants