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

add output cypher component #118

Merged
merged 12 commits into from
Nov 6, 2024
Merged

add output cypher component #118

merged 12 commits into from
Nov 6, 2024

Conversation

jem-davies
Copy link
Collaborator

No description provided.

@jem-davies jem-davies mentioned this pull request Sep 17, 2024
Open
gregfurman
gregfurman requested changes Sep 17, 2024
View reviewed changes
cmd/bento/config.yaml Outdated Show resolved Hide resolved
internal/impl/cypher/output_cypher.go Outdated Show resolved Hide resolved
internal/impl/cypher/output_cypher.go Show resolved Hide resolved
internal/impl/cypher/output_cypher.go Outdated Show resolved Hide resolved
@jem-davies
add output cypher component
8ddfa45 
Signed-off-by: Jem Davies <jemsot@gmail.com>
@jem-davies
add basic_auth to cypher output
6d77746 
Signed-off-by: Jem Davies <jemsot@gmail.com>
@jem-davies
fix for linter, make docs
6641275 
Signed-off-by: Jem Davies <jemsot@gmail.com>
@jem-davies
add integration tests, add check for query and values
b9b05f5 
Signed-off-by: Jem Davies <jemsot@gmail.com>
@jem-davies jem-davies marked this pull request as ready for review September 28, 2024 19:35
gregfurman
gregfurman requested changes Sep 29, 2024
View reviewed changes
Copy link
Collaborator

@gregfurman gregfurman left a comment

Choose a reason for hiding this comment

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

Looking good. Have left some comments and nits. Haven't validated the tests on my local though but will do so soon.

internal/impl/cypher/output_cypher.go Outdated Show resolved Hide resolved
internal/impl/cypher/output_cypher_test.go Show resolved Hide resolved
internal/impl/cypher/output_cypher.go Outdated Show resolved Hide resolved
internal/impl/cypher/output_cypher.go Outdated Show resolved Hide resolved
website/docs/components/outputs/cypher.md Outdated Show resolved Hide resolved
internal/impl/cypher/output_cypher.go Show resolved Hide resolved
@jem-davies
refactor tests
e2e8baf 
Signed-off-by: Jem Davies <jemsot@gmail.com>
@jem-davies
refactor tests, update for feedback on PR
5941b6e 
Signed-off-by: Jem Davies <jemsot@gmail.com>
@jem-davies
fix version
e6f859e 
Signed-off-by: Jem Davies <jemsot@gmail.com>
@jem-davies jem-davies requested a review from gregfurman October 2, 2024 21:00
@jem-davies
Copy link
Collaborator Author

@gregfurman thanks for review - updated the code / responded to your comment...

gregfurman
gregfurman reviewed Oct 5, 2024
View reviewed changes
Copy link
Collaborator

@gregfurman gregfurman left a comment

Choose a reason for hiding this comment

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

This is looking great, dude! Just some comments regarding tests (setup + teardown, properly passing around the testing.T and adding a batching test).

Also ran your tests and my local and all looks good 👍

internal/impl/cypher/output_cypher.go Outdated Show resolved Hide resolved
internal/impl/cypher/output_cypher.go Show resolved Hide resolved
internal/impl/cypher/output_cypher_test.go Show resolved Hide resolved
internal/impl/cypher/output_cypher_test.go Outdated Show resolved Hide resolved
internal/impl/cypher/output_cypher_test.go Outdated Show resolved Hide resolved
internal/impl/cypher/output_cypher_test.go Outdated Show resolved Hide resolved
internal/impl/cypher/output_cypher_test.go Outdated Show resolved Hide resolved
internal/impl/cypher/output_cypher.go Show resolved Hide resolved
internal/impl/cypher/output_cypher_test.go Show resolved Hide resolved
@jem-davies
cypherOutputDescription format
c05aa05 
Signed-off-by: Jem Davies <jemsot@gmail.com>
@jem-davies
rename badly named variables
33d636a 
Signed-off-by: Jem Davies <jemsot@gmail.com>
@jem-davies
add cleanup function to tests, fix test context
b10b636 
Signed-off-by: Jem Davies <jemsot@gmail.com>
@jem-davies
fix batching + add test
c3f5fb1 
Signed-off-by: Jem Davies <jemsot@gmail.com>
@jem-davies
fix linter
938da75 
Signed-off-by: Jem Davies <jemsot@gmail.com>
@jem-davies jem-davies requested a review from gregfurman October 6, 2024 13:37
@jem-davies
Copy link
Collaborator Author

@gregfurman addressed the last batch of feedback, thanks

gregfurman
gregfurman approved these changes Oct 31, 2024
View reviewed changes
Copy link
Collaborator

@gregfurman gregfurman left a comment

Choose a reason for hiding this comment

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

LGTM! Sorry for taking so long on this one 😄

@jem-davies jem-davies merged commit 30ef9d9 into main Nov 6, 2024
3 checks passed
@jem-davies jem-davies deleted the cypher-connectors branch November 6, 2024 00:06
jem-davies added a commit that referenced this pull request Nov 7, 2024
@jem-davies
add output cypher component (#118)
d16dcd5 
* add output cypher component

Signed-off-by: Jem Davies <jemsot@gmail.com>
jem-davies added a commit that referenced this pull request Nov 7, 2024
@jem-davies
add output cypher component (#118)
bf03539 
* add output cypher component

Signed-off-by: Jem Davies <jemsot@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gregfurman gregfurman gregfurman approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jem-davies @gregfurman