Skip to content
This repository has been archived by the owner on Jan 27, 2021. It is now read-only.

Upgrade micro/cli to v2 #22

Merged
merged 6 commits into from
Feb 4, 2020
Merged

Upgrade micro/cli to v2 #22

merged 6 commits into from
Feb 4, 2020

Conversation

DeepDiver1975
Copy link
Member

No description provided.

@DeepDiver1975 DeepDiver1975 self-assigned this Jan 29, 2020
@tboerger
Copy link
Contributor

Drone fails

@DeepDiver1975
Copy link
Member Author

Drone fails

I know ... that's why it's a draft pr ... we need an upstream release of micro v2 first ... will come these days ....

@C0rby
Copy link
Contributor

C0rby commented Feb 3, 2020

I basically just copied the code for Read and Stream from go-micro. It's the simplest solution I guess but if it's bad for our use case e.g. because of the pessimistic locking in the buffer then I'll rewrite it.

@C0rby C0rby requested a review from refs February 3, 2020 16:23
Copy link
Member

@refs refs left a comment

Choose a reason for hiding this comment

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

lgtm 👍 but a couple of remarks. Having a look at go-micro and that they added a structured in memory logger, we could use it and get rid of our wrapper; an experimental branch could be opened to explore this path, as we basically copied the memory log interface implementation to satisfy the contract.

If we get this merged you'd need to provide a changelog. Have a look at the changelog folder for steps to follow. Other than that, flawless /cc @C0rby

@C0rby C0rby marked this pull request as ready for review February 4, 2020 11:06
@ownclouders
Copy link

Codacy Here is an overview of what got changed by this pull request:

Complexity increasing per file
==============================
- log/stream.go  3
         

See the complete overview on Codacy

@C0rby C0rby merged commit 8f9564d into master Feb 4, 2020
@delete-merged-branch delete-merged-branch bot deleted the bugfix/upgrade-micro/cli branch February 4, 2020 14:04
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants