Skip to content

[Cosmos] Migrate get_document to pipelines architecture #357

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

Merged
merged 4 commits into from
Sep 7, 2021

Conversation

eholk
Copy link
Contributor

@eholk eholk commented Sep 1, 2021

This migrates get_document to the new pipelines architecture and updates all the examples to compile.

I'm sure this can be improved, so I'd definitely appreciate code review feedback. Also, although the examples compile, I didn't run them so I'm not 100% sure this preserves the previous behavior. Is there a standard approach people have to testing these things?

Issue #290

eholk added 2 commits August 31, 2021 17:03
This change begins the migration process for moving get_document to the
new pipelines architecture. Most of the code is moved over now, but the
example programs still need updated.

Issue Azure#290
Copy link
Contributor

@rylev rylev left a comment

Choose a reason for hiding this comment

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

Thanks! Looks great. We don't have a good way of testing right now (that's in the works with #348) so we'll need to test this manually.

I only had one comment on some dead code we can remove.

Overall, I think it might make sense for us to merge this and then when #348 we can ensure it's in working order.

@JoshGendein
Copy link
Member

Not sure what others are doing but I just created a cosmos account in a personal subscription and test manually. Also because the e2e tests are behind a feature don't forget to check them with cargo check --tests --features test_e2e. I think you can add this as config to rust-analyzer.

@eholk
Copy link
Contributor Author

eholk commented Sep 3, 2021

@JoshGendein - Thanks for the suggestion. I found some more code that needs to be updated by doing that. I'll try doing the e2e tests on a personal subscription too.

@eholk eholk force-pushed the cosmos-get-document branch from eff95f3 to 10068ab Compare September 3, 2021 22:20
@eholk
Copy link
Contributor Author

eholk commented Sep 3, 2021

Okay, I tried running the tests on a personal subscription. I had failures on create_and_delete_document as well as replace_document, but I get similar failures on the main branch, so it doesn't seem like my change broke it.

@rylev
Copy link
Contributor

rylev commented Sep 7, 2021

♥ Thank you!

@rylev rylev merged commit 0bc8af3 into Azure:main Sep 7, 2021
@eholk eholk deleted the cosmos-get-document branch September 7, 2021 21:16
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