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

Consul catalog provider #56

Merged
merged 2 commits into from
May 13, 2021
Merged

Consul catalog provider #56

merged 2 commits into from
May 13, 2021

Conversation

negasus
Copy link
Contributor

@negasus negasus commented Apr 23, 2021

Implement consul catalog provider

Issue #10

@umputun
Copy link
Owner

umputun commented Apr 23, 2021 •
edited
Loading

As it marked as a draft and review wasn't requested I haven't reviewed it for real, just took a quick look. I glad to see you have not used some SDK/library for this and kept it all with stdlib, I like it a lot. The code looks clean and readable. I notice just a few minor things, like TrimRight instead of TrimSuffix you probably meant.

thx

@negasus negasus changed the title Consul catalog profiver Consul catalog provider Apr 24, 2021
@negasus
Copy link
Contributor Author

negasus commented Apr 24, 2021

I use TrimRight for 'path//' cases

@negasus negasus marked this pull request as ready for review April 24, 2021 07:47
@negasus negasus requested a review from umputun as a code owner April 24, 2021 07:47
@negasus
Copy link
Contributor Author

negasus commented Apr 24, 2021

Oops, I have to add documentation.

umputun
umputun requested changes Apr 24, 2021
View reviewed changes
Copy link
Owner

@umputun umputun left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I have not analyzed what this code is doing but rather how it's doing things. I think it can be simplified and be much easier to digest

app/discovery/provider/consulcatalog/list.go Outdated Show resolved Hide resolved
app/discovery/provider/consulcatalog/list.go Outdated Show resolved Hide resolved
app/discovery/provider/consulcatalog/list.go Outdated Show resolved Hide resolved
app/discovery/provider/consulcatalog/consulcatalog.go Outdated Show resolved Hide resolved
app/discovery/provider/consulcatalog/consulcatalog.go Outdated Show resolved Hide resolved
app/discovery/provider/consulcatalog/consulcatalog.go Outdated Show resolved Hide resolved
app/discovery/provider/consulcatalog/consulcatalog.go Outdated Show resolved Hide resolved
app/discovery/provider/consulcatalog/client.go Outdated Show resolved Hide resolved
app/discovery/provider/consulcatalog/consulcatalog.go Show resolved Hide resolved
app/discovery/provider/consulcatalog/consulcatalog.go Outdated Show resolved Hide resolved
@umputun
Copy link
Owner

umputun commented Apr 24, 2021

see also https://github.com/umputun/reproxy/pull/56/checks?check_run_id=2426021843

running the same linter locally is a good way to catch all of them before it hits CI

@negasus negasus requested a review from umputun April 28, 2021 08:43
umputun
umputun requested changes Apr 29, 2021
View reviewed changes
Copy link
Owner

@umputun umputun left a comment

Choose a reason for hiding this comment

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

I don't want to repeat all remarks I made before, but many of them not even touched

app/discovery/provider/consulcatalog/consulcatalog.go Outdated Show resolved Hide resolved
app/discovery/provider/consulcatalog/consulcatalog.go Outdated Show resolved Hide resolved
@umputun umputun mentioned this pull request May 5, 2021
Closed
@negasus negasus requested a review from umputun May 7, 2021 10:29
umputun
umputun approved these changes May 10, 2021
View reviewed changes
Copy link
Owner

@umputun umputun left a comment

Choose a reason for hiding this comment

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

LGTM

Will be nice if you can fix the linter warn

@umputun
Copy link
Owner

umputun commented May 10, 2021

one more thing - pls rebase it to a single commit or a few logically isolated commits.

@umputun umputun merged commit 01ae284 into umputun:master May 13, 2021
@negasus negasus deleted the feature/consul-provider branch May 13, 2021 08:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@umputun umputun umputun 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.

None yet
2 participants
@negasus @umputun