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

MF-1340 - Add CLI config TOML file #1858

Merged
merged 58 commits into from
Aug 8, 2023
Merged

Conversation

WashingtonKK
Copy link
Contributor

@WashingtonKK WashingtonKK commented Jul 17, 2023

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?

Creates a TOML file for CLI

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

#1340

List any changes that modify/break current functionality

N/A

Have you included tests for your changes?

No

Did you document any new/modified functionality?

No

Notes

@WashingtonKK WashingtonKK requested a review from a team as a code owner July 17, 2023 10:08
@codecov
Copy link

codecov bot commented Jul 17, 2023

Codecov Report

Merging #1858 (79d61bc) into master (ca52a5a) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1858   +/-   ##
=======================================
  Coverage   67.43%   67.43%           
=======================================
  Files         118      118           
  Lines        9491     9491           
=======================================
  Hits         6400     6400           
  Misses       2409     2409           
  Partials      682      682           

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

@SammyOina
Copy link
Contributor

@WashingtonKK please refer to the correct originating issue

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.

There is no support in the CLI for parsing most of the config parameters in this PR https://github.com/mainflux/mainflux/blob/aee0081864f9654f0eeb2c08a36ed5350ce04372/cli/config.go#L15-L20.
I also don't think this PR fully addresses the requirements in #1340

@WashingtonKK
Copy link
Contributor Author

There is no support in the CLI for parsing most of the config parameters in this PR

https://github.com/mainflux/mainflux/blob/aee0081864f9654f0eeb2c08a36ed5350ce04372/cli/config.go#L15-L20

.
I also don't think this PR fully addresses the requirements in #1340

What do you recommend?

Also, @dborovcanin , what do you think?

Copy link
Collaborator

@dborovcanin dborovcanin left a comment

Choose a reason for hiding this comment

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

There is no need to put "flags" and "flags.root", you can use "urls" for root URLs. The same goes for "flags.channels" and "flags.client" - "channel" and "client" are sufficient. Also, rename client to filter or something similar. Once that's done, you need to add the actual parsing and set the CLI config values from this file, like @SammyOina mentioned here.

@dborovcanin
Copy link
Collaborator

There is no support in the CLI for parsing most of the config parameters in this PR
https://github.com/mainflux/mainflux/blob/aee0081864f9654f0eeb2c08a36ed5350ce04372/cli/config.go#L15-L20

.
I also don't think this PR fully addresses the requirements in #1340

What do you recommend?

Also, @dborovcanin , what do you think?

Addressed in PR review.

cli/config.go Outdated Show resolved Hide resolved
cli/config.go Outdated Show resolved Hide resolved
cli/config.go Outdated Show resolved Hide resolved
cli/config.toml Outdated Show resolved Hide resolved
cli/config.go Outdated Show resolved Hide resolved
cli/config.go Outdated Show resolved Hide resolved
cli/config.go Outdated Show resolved Hide resolved
cli/config.go Outdated Show resolved Hide resolved
cli/config.go Outdated Show resolved Hide resolved
cli/config.go Outdated Show resolved Hide resolved
cli/config.go Outdated Show resolved Hide resolved
cli/config.go Outdated Show resolved Hide resolved
cli/config.go Outdated Show resolved Hide resolved
cli/config.go Outdated Show resolved Hide resolved
cli/config.go Outdated Show resolved Hide resolved
cli/config.go Show resolved Hide resolved
cli/config.go Outdated Show resolved Hide resolved
cli/config.go Outdated Show resolved Hide resolved
cli/config.go Outdated Show resolved Hide resolved
cli/config.go Outdated Show resolved Hide resolved
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.

For all cases utilizing the fmt and log package to output text use the mainflux logger package instead

cli/config.go Outdated Show resolved Hide resolved
cli/config.go Outdated Show resolved Hide resolved
cli/config.go Outdated Show resolved Hide resolved
cli/config.go Outdated Show resolved Hide resolved
docker/.env Outdated Show resolved Hide resolved
cli/config.go Outdated Show resolved Hide resolved
cli/config.go Outdated Show resolved Hide resolved
@arvindh123
Copy link
Contributor

@WashingtonKK , Configurations in config.toml are not getting configured in SDK,
I tried with users URL it doesn't works

@WashingtonKK
Copy link
Contributor Author

@WashingtonKK , Configurations in config.toml are not getting configured in SDK, I tried with users URL it doesn't works

@arvindh123 , I have fixed this

cmd/cli/main.go Outdated Show resolved Hide resolved
cli/utils.go Outdated Show resolved Hide resolved
cli/config.toml Outdated Show resolved Hide resolved
cli/config.toml Outdated Show resolved Hide resolved
cli/config.toml Outdated Show resolved Hide resolved
cli/config.go Outdated Show resolved Hide resolved
cli/config.go Outdated Show resolved Hide resolved
cli/config.go Outdated Show resolved Hide resolved
cli/config.go Outdated Show resolved Hide resolved
cli/utils.go Outdated Show resolved Hide resolved
cli/utils.go Show resolved Hide resolved
cli/config.go Outdated Show resolved Hide resolved
configs/cli/config.toml Outdated Show resolved Hide resolved
configs/cli/config.toml Outdated Show resolved Hide resolved
configs/cli/config.toml Outdated Show resolved Hide resolved
Signed-off-by: WashingtonKK <washingtonkigan@gmail.com>
Signed-off-by: WashingtonKK <washingtonkigan@gmail.com>
Signed-off-by: WashingtonKK <washingtonkigan@gmail.com>
cli/config.go Outdated Show resolved Hide resolved
Signed-off-by: WashingtonKK <washingtonkigan@gmail.com>
cli/config.go Outdated Show resolved Hide resolved
cli/config.go Outdated Show resolved Hide resolved
cli/config.go Show resolved Hide resolved
Signed-off-by: WashingtonKK <washingtonkigan@gmail.com>
cli/config.toml Outdated Show resolved Hide resolved
cli/config.go Outdated Show resolved Hide resolved
cli/config.go Outdated Show resolved Hide resolved
Signed-off-by: WashingtonKK <washingtonkigan@gmail.com>
Signed-off-by: WashingtonKK <washingtonkigan@gmail.com>
cli/config.go Outdated Show resolved Hide resolved
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>
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.

LGTM

Copy link
Collaborator

@dborovcanin dborovcanin left a comment

Choose a reason for hiding this comment

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

Remove this since it is created automatically anyway when you run the CLI.

Signed-off-by: WashingtonKK <washingtonkigan@gmail.com>
Signed-off-by: WashingtonKK <washingtonkigan@gmail.com>
config.toml 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 0f0d761 into absmach:master Aug 8, 2023
3 checks passed
@WashingtonKK WashingtonKK deleted the MF-1340 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.

7 participants