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

NOISSUE - Change import name aliases #1868

Merged
merged 21 commits into from
Aug 11, 2023

Conversation

WashingtonKK
Copy link
Contributor

Pull request title should be MF-XXX - description or NOISSUE - description where XXX is ID of issue that this PR relate to.
Please review the CONTRIBUTING.md file for detailed contributing guidelines.

What does this do?

Currently, in main.go files we have Go package import aliases that are not according to Go recommended practices. This PR changes the aliases of the imports to correspond to go's recommended practices.

Which issue(s) does this PR fix/relate to?

No issue

List any changes that modify/break current functionality

N/A

Have you included tests for your changes?

N/A

Did you document any new/modified functionality?

Notes

@WashingtonKK WashingtonKK requested a review from a team as a code owner July 23, 2023 19:35
@codecov
Copy link

codecov bot commented Jul 23, 2023

Codecov Report

Merging #1868 (1ed110c) into master (91e3873) will not change coverage.
The diff coverage is 69.23%.

@@           Coverage Diff           @@
##           master    #1868   +/-   ##
=======================================
  Coverage   67.16%   67.16%           
=======================================
  Files         118      118           
  Lines        9076     9076           
=======================================
  Hits         6096     6096           
  Misses       2351     2351           
  Partials      629      629           
Files Changed Coverage Δ
consumers/notifiers/api/responses.go 100.00% <ø> (ø)
consumers/notifiers/api/transport.go 91.75% <ø> (ø)
consumers/notifiers/postgres/subscriptions.go 81.39% <ø> (ø)
consumers/notifiers/service.go 60.00% <ø> (ø)
consumers/writers/influxdb/consumer.go 89.87% <ø> (ø)
consumers/writers/mongodb/consumer.go 79.31% <ø> (ø)
http/api/transport.go 71.55% <ø> (ø)
mqtt/forwarder.go 0.00% <ø> (ø)
pkg/errors/sdk_errors.go 0.00% <ø> (ø)
pkg/groups/postgres/groups.go 68.81% <ø> (ø)
... and 17 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Contributor

@SammyOina SammyOina left a comment

Choose a reason for hiding this comment

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

Check for instances where an alias is used but not required and rebase branch to sync with master

Copy link
Member

@rodneyosodo rodneyosodo left a comment

Choose a reason for hiding this comment

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

also change in other services for example bootstrap/postgres/setup_test.go

@@ -16,10 +16,10 @@ import (
"github.com/mainflux/mainflux/consumers"
consumerTracing "github.com/mainflux/mainflux/consumers/tracing"
Copy link
Contributor

Choose a reason for hiding this comment

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

consumertracing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Solved

@WashingtonKK WashingtonKK force-pushed the 257-change-import-aliases-naming branch 2 times, most recently from 03956f4 to 52c09af Compare July 24, 2023 17:15
@rodneyosodo
Copy link
Member

rodneyosodo commented Jul 24, 2023

Check for instances where an alias is used but not required and rebase branch to sync with master

Did you check this? For example in bootstrap/postgres/setup_test.go you don't need dockertest "github.com/ory/dockertest/v3" as the package name is dockertest also in cli/utils.go

  • cmd/influxdb-reader/main.go - influxdb2

Copy link
Member

@rodneyosodo rodneyosodo left a comment

Choose a reason for hiding this comment

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

Fix

  • cmd/lora/main.go redis
  • consumerTracing

Copy link
Contributor

@arvindh123 arvindh123 left a comment

Choose a reason for hiding this comment

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

In the following file pgClient is used, Please change them too
users/groups/postgres/setup_test.go
users/policies/postgres/setup_test.go

@WashingtonKK
Copy link
Contributor Author

@arvindh123 , I have addressed the commenrts

drasko
drasko previously approved these changes Jul 25, 2023
Copy link
Contributor

@drasko drasko left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@rodneyosodo rodneyosodo left a comment

Choose a reason for hiding this comment

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

Copy link
Member

@rodneyosodo rodneyosodo left a comment

Choose a reason for hiding this comment

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

enabled --enable goimports --enable gci on golangcilint

mteodor
mteodor previously approved these changes Jul 26, 2023
@WashingtonKK WashingtonKK dismissed stale reviews from mteodor and drasko via 2455b5e July 26, 2023 19:21
drasko
drasko previously approved these changes Jul 26, 2023
Copy link
Contributor

@drasko drasko left a comment

Choose a reason for hiding this comment

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

LGTM

cmd/lora/main.go Outdated
@@ -11,7 +11,7 @@ import (
"os"
"time"

mqttPaho "github.com/eclipse/paho.mqtt.golang"
mqttpaho "github.com/eclipse/paho.mqtt.golang"
r "github.com/go-redis/redis/v8"
Copy link
Contributor

Choose a reason for hiding this comment

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

the alias "r" is very vague

Copy link
Contributor

Choose a reason for hiding this comment

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

same case in cmd/opcua/main.go

@ianmuchyri
Copy link
Contributor

ianmuchyri commented Jul 27, 2023

@ianmuchyri
Copy link
Contributor

ianmuchyri commented Jul 27, 2023

Copy link
Contributor

@SammyOina SammyOina left a comment

Choose a reason for hiding this comment

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

check for other instances where an alias is used but not required

Comment on lines 12 to 13
bootstraprepo "github.com/mainflux/mainflux/bootstrap/postgres"
pgclient "github.com/mainflux/mainflux/internal/clients/postgres"
Copy link
Contributor

Choose a reason for hiding this comment

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

only one of these requires an alias, see other instances in other files where an alias is used but not required

@@ -10,7 +10,7 @@ import (

"github.com/jmoiron/sqlx"
"github.com/mainflux/mainflux/certs/postgres"
pgClient "github.com/mainflux/mainflux/internal/clients/postgres"
pgclient "github.com/mainflux/mainflux/internal/clients/postgres"
"github.com/mainflux/mainflux/logger"
dockertest "github.com/ory/dockertest/v3"
Copy link
Contributor

Choose a reason for hiding this comment

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

doesn't require an alias

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does, we have:
"github.com/mainflux/mainflux/certs/postgres"
and
pgClient "github.com/mainflux/mainflux/internal/clients/postgres"

Comment on lines 22 to 23
authclient "github.com/mainflux/mainflux/internal/clients/grpc/auth"
jaegerclient "github.com/mainflux/mainflux/internal/clients/jaeger"
Copy link
Contributor

Choose a reason for hiding this comment

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

are these aliases required

authclient "github.com/mainflux/mainflux/internal/clients/grpc/auth"
jaegerclient "github.com/mainflux/mainflux/internal/clients/jaeger"
pgclient "github.com/mainflux/mainflux/internal/clients/postgres"
redisclient "github.com/mainflux/mainflux/internal/clients/redis"
Copy link
Contributor

Choose a reason for hiding this comment

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

same for this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

pgclient and redis client are needed

bootstrapRepo "github.com/mainflux/mainflux/bootstrap/postgres"
pgClient "github.com/mainflux/mainflux/internal/clients/postgres"
"github.com/mainflux/mainflux/bootstrap/postgres"
pgclient "github.com/mainflux/mainflux/internal/clients/postgres"
"github.com/mainflux/mainflux/logger"

dockertest "github.com/ory/dockertest/v3"
Copy link
Member

Choose a reason for hiding this comment

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

dockertest "github.com/ory/dockertest/v3"

This is not needed as the package is dockertest. This applies to all occurrences

@WashingtonKK WashingtonKK force-pushed the 257-change-import-aliases-naming branch 3 times, most recently from a26f461 to f0d9597 Compare July 28, 2023 14:14
@WashingtonKK WashingtonKK force-pushed the 257-change-import-aliases-naming branch 2 times, most recently from 978293f to 6a45d30 Compare August 10, 2023 19:18
Signed-off-by: WashingtonKK <washingtonkigan@gmail.com>
Signed-off-by: WashingtonKK <washingtonkigan@gmail.com>
Signed-off-by: WashingtonKK <washingtonkigan@gmail.com>
Signed-off-by: WashingtonKK <washingtonkigan@gmail.com>

Fix aliases

Signed-off-by: WashingtonKK <washingtonkigan@gmail.com>

FIx errors

Signed-off-by: WashingtonKK <washingtonkigan@gmail.com>

Fix error

Signed-off-by: WashingtonKK <washingtonkigan@gmail.com>

FIx merge

Signed-off-by: WashingtonKK <washingtonkigan@gmail.com>

FIx merge

Signed-off-by: WashingtonKK <washingtonkigan@gmail.com>

FIx merge

Signed-off-by: WashingtonKK <washingtonkigan@gmail.com>
Signed-off-by: WashingtonKK <washingtonkigan@gmail.com>
Signed-off-by: WashingtonKK <washingtonkigan@gmail.com>
Signed-off-by: WashingtonKK <washingtonkigan@gmail.com>
Signed-off-by: WashingtonKK <washingtonkigan@gmail.com>
Signed-off-by: WashingtonKK <washingtonkigan@gmail.com>
Signed-off-by: WashingtonKK <washingtonkigan@gmail.com>
Signed-off-by: WashingtonKK <washingtonkigan@gmail.com>
Signed-off-by: WashingtonKK <washingtonkigan@gmail.com>
Signed-off-by: WashingtonKK <washingtonkigan@gmail.com>
Signed-off-by: WashingtonKK <washingtonkigan@gmail.com>
Signed-off-by: WashingtonKK <washingtonkigan@gmail.com>
@WashingtonKK WashingtonKK force-pushed the 257-change-import-aliases-naming branch from 9ee3fde to c80875c Compare August 10, 2023 19:47
Signed-off-by: WashingtonKK <washingtonkigan@gmail.com>
Signed-off-by: WashingtonKK <washingtonkigan@gmail.com>
Signed-off-by: WashingtonKK <washingtonkigan@gmail.com>
.semaphore/semaphore.yml Outdated Show resolved Hide resolved
mqtt/handler.go Outdated Show resolved Hide resolved
scripts/ci.sh Outdated Show resolved Hide resolved
Signed-off-by: WashingtonKK <washingtonkigan@gmail.com>
Signed-off-by: WashingtonKK <washingtonkigan@gmail.com>
.semaphore/semaphore.yml Outdated Show resolved Hide resolved
Signed-off-by: WashingtonKK <washingtonkigan@gmail.com>
Copy link
Contributor

@drasko drasko left a comment

Choose a reason for hiding this comment

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

LGTM

@drasko drasko merged commit e2992cb into absmach:master Aug 11, 2023
4 checks passed
@WashingtonKK WashingtonKK deleted the 257-change-import-aliases-naming branch October 21, 2023 06:55
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.

8 participants