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

Simplify CLI commands by saving rpc-addr and project to context #592

Closed
wants to merge 12 commits into from

Conversation

Wu22e
Copy link
Contributor

@Wu22e Wu22e commented Jul 30, 2023

What this PR does / why we need it:
I have made changes to manage the rpc-addr and insecure global flags in the ~/.yorkie/config.json in the CLI application. In this process, the viper library was used to bind the necessary flag values to viper variables. Moreover, we were able to apply a priority as to which should be read between the user's explicit flag input and the config value.
(The explicit flag is bound to the viper variables before the config value.)

Which issue(s) this PR fixes:

Fixes #544

Special notes for your reviewer:

Does this PR introduce a user-facing change?:


Additional documentation:


This pull request is failing the TestAdminRPCServerBackend/admin_signup_test. I am unable to find the cause of the failure, so I'm asking for help with the issue along with the pull request.

Checklist:

  • Added relevant tests or not required
  • Didn't break anything

@CLAassistant
Copy link

CLAassistant commented Jul 30, 2023

CLA assistant check
All committers have signed the CLA.

@hackerwins hackerwins marked this pull request as draft July 30, 2023 23:32
@hackerwins hackerwins self-requested a review July 30, 2023 23:41
Copy link
Member

@hackerwins hackerwins left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution! I have a couple of comments:

A. Failure of admin_signup_test in CI

In this PR, I noticed that there are some module updates, and it appears that changing the status code to grpc v1.55.0 is causing the failure in the admin_signup_test. To address this, how about keeping previous versions and only retaining the viper installation?

B. CLI user flow

It's important to define the user flow for those using the CLI. Could you provide some examples of CLI usage following this flow:

- Login, Logout
- Login, Switch context with another Login, Logout
- Login, Switch context with another Login, Switch back to the previous context, 
 Logout

Additionally, we should also verify if these CLI changes have any side effects that need to be checked.

@Wu22e Wu22e force-pushed the save-rpc-addr-to-context branch from fb87da3 to d51024f Compare September 3, 2023 18:57
@krapie krapie self-requested a review September 4, 2023 03:26
@Wu22e Wu22e force-pushed the save-rpc-addr-to-context branch from 7e40b95 to d51024f Compare September 4, 2023 07:39
@Wu22e Wu22e force-pushed the save-rpc-addr-to-context branch from d51024f to 4e36603 Compare September 4, 2023 07:42
@Wu22e
Copy link
Contributor Author

Wu22e commented Sep 4, 2023

Sorry for the late response.
Thank you for your comment.

A. Failure of admin_signup_test in CI

Firstly, I was able to solve the problem by downgrading the grpc version to 1.54.0. However, I have a doubt regarding the actual response code value of the test code. This test code throws the 'Unknown' rpc error codes when the same user tries to sign up. (from google.golang.org/grpc/codes)

In fact, when adding the viper library, if the grpc version is upgraded to 1.55.0, the actual value of this test is not 'Unknown' but throws an 'Internal' error code.

I think it would be better to the test's intent to use the 'AlreadyExists' code provided by grpc codes. I'm curious about your opinion on this! (For the SignUp function process, errors related to duplicate keys should be wrapped with the 'AlreadyExists' code.)

B. CLI user flow
Initially, not only the 'auths' map field was added to the Config structure, but also fields like 'rpcAddr' and 'isInsecure'.

Every time a user logs in, they must specify the 'rpcAddr' and 'isInsecure' values as global flags. (mapping : --rpc-addr -> rpcAddr , --insecure -> isInsecure) If you log in with a new 'rpcAddr', the entry in 'auths' increases.

'Context' sub-command has been added.
ex) yorkie context ls -> It displays the 'auths', 'rpcaddr', and 'isInsecure' values set in config.json.

CASE 1 : login -> logout

[login]
yorkie login -u admin -p admin --rpc-addr localhost:11101 --insecure

-> Result of config.json 
{"auths":{"localhost:11101":"eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJleHAiOjE2OTQ0NDA5MzQsInVzZXJuYW1lIjoiYWRtaW4ifQ.svxKTUC0LikGw86QtNd8Zr_JQaFNu3CEDUPmGXru5wY"},"rpcAddr":"localhost:11101","isInsecure":true}
[logout]
yorkie logout

-> Result of config.json
deleted

CASE 2 : login -> switch context with another login -> logout

[login] -- (1)
yorkie login -u admin -p admin --rpc-addr localhost:11101 --insecure

-> Result of config.json 
{"auths":{"localhost:11101":"eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJleHAiOjE2OTQ0NDA5MzQsInVzZXJuYW1lIjoiYWRtaW4ifQ.svxKTUC0LikGw86QtNd8Zr_JQaFNu3CEDUPmGXru5wY"},"rpcAddr":"localhost:11101","isInsecure":true}

[context ls] -- (2)
yorkie context ls

-> Current Context: rpcAddr: api.yorkie.dev:443 isInsecure: true
[switch context with another login] -- (3)
yorkie login -u admin -p admin --rpc-addr localhost:11101 —insecure

-> Result of config.json
{"auths":{"api.yorkie.dev:443":"eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJleHAiOjE2OTQ0NDM4MTcsInVzZXJuYW1lIjoiYWRtaW4ifQ.ERmYBT08DOGiP6tjjl0iWxH8QPk4O1mau_E3OzBvYy8","localhost:11101":"eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJleHAiOjE2OTQ0NDM4NDIsInVzZXJuYW1lIjoiYWRtaW4ifQ.b-CZuufBaPMyMBIA6vaR66EdUzdSnu1hVCveGx6zCRw"},"rpcAddr":"localhost:11101","isInsecure":true}
[context ls] -- (4)
yorkie context ls

-> Current Context: rpcAddr: localhost:11101 isInsecure: true
[logout]
yorkie logout

-> Result of config.json
{"auths":{"api.yorkie.dev:443":"eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJleHAiOjE2OTQ0NDM4MTcsInVzZXJuYW1lIjoiYWRtaW4ifQ.ERmYBT08DOGiP6tjjl0iWxH8QPk4O1mau_E3OzBvYy8"},"rpcAddr":"","isInsecure":false}
[context ls]
yorkie context ls

-> Current Context: rpcAddr: isInsecure: false

CASE 3 : login -> switch context with another login -> switch back to the previous context -> logout

In this case, the processes (1), (2), (3), and (4) in CASE 2 are the same.

[switch back to the previous context]
yorkie context set --rpc-addr api.yorkie.dev:443

-> Result of config.json
{"auths":{"api.yorkie.dev:443":"eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJleHAiOjE2OTQ0NDQ4MzEsInVzZXJuYW1lIjoiYWRtaW4ifQ.mmsSQcwACB49uBtYb04kcQeY4GO6K8CS5qV1p4GEBXs","localhost:11101":"eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJleHAiOjE2OTQ0NDQ1MTAsInVzZXJuYW1lIjoiYWRtaW4ifQ.IzAlxrft9lFYdvw246LmdG0h3nkaPrNkb1NLYn8F1eg"},"rpcAddr":"api.yorkie.dev:443","isInsecure":false}

[context ls]
yorkie context ls

-> Current Context: rpcAddr: api.yorkie.dev:443 isInsecure: false
[logout]
yorkie logout

-> Result of config.json
{"auths":{"localhost:11101":"eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJleHAiOjE2OTQ0NDQ1MTAsInVzZXJuYW1lIjoiYWRtaW4ifQ.IzAlxrft9lFYdvw246LmdG0h3nkaPrNkb1NLYn8F1eg"},"rpcAddr":"","isInsecure":false}
[context ls]
yorkie context ls

-> Current Context: rpcAddr: isInsecure: false

If you find any issues about user flow, I would appreciate your feedback!


Regarding the last comment, you mentioned the need to check for side effects due to changes in the CLI. Could you provide specific guidance?

Additionally, before this Pull Request gets merged, it seems necessary to update the CLI documentation on the yorkie-team.github.io. Once the discussion about the contents of this pull request is concluded, should I create an issue and then proceed with the work?

@hackerwins
Copy link
Member

hackerwins commented Sep 5, 2023

Thank you for the detailed description. But first, please proceed with the following.

A. Fix failure in CI

B. Convert Draft to Ready

  • Once you resolve the CI issue, please change Draft to Ready.
  • We will review this PR.

https://github.com/yorkie-team/yorkie/blob/main/CONTRIBUTING.md#contribution-flow

@Wu22e Wu22e marked this pull request as ready for review September 5, 2023 16:03
@Wu22e Wu22e requested a review from hackerwins September 6, 2023 10:18
Copy link
Member

@krapie krapie left a comment

Choose a reason for hiding this comment

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

I left one small comment briefly.

cmd/yorkie/context/list.go Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Sep 9, 2023

Codecov Report

Patch coverage has no change and project coverage change: +0.07% 🎉

Comparison is base (b66068d) 49.40% compared to head (2dea9fd) 49.47%.
Report is 4 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #592      +/-   ##
==========================================
+ Coverage   49.40%   49.47%   +0.07%     
==========================================
  Files          69       69              
  Lines        9917     9951      +34     
==========================================
+ Hits         4899     4923      +24     
- Misses       4504     4512       +8     
- Partials      514      516       +2     

see 4 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Wu22e Wu22e requested a review from krapie September 9, 2023 06:17
Copy link
Member

@krapie krapie left a comment

Choose a reason for hiding this comment

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

How about adding name field in config, and use name instead of rpcAddr for context commands to easily recognize and switch between contexts?

Ref: https://kubernetes.io/ko/docs/tasks/access-application-cluster/configure-access-multiple-clusters/

os.Exit(1)
}

file, err := os.Open(filepath.Clean(configPathValue))
Copy link
Member

Choose a reason for hiding this comment

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

What does this file do?
Is this required for viper.ReadInConfig()?

Copy link
Contributor Author

@Wu22e Wu22e Sep 11, 2023

Choose a reason for hiding this comment

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

I had some concerns about this part.
Actually, I don't use the file variable in the section you mentioned.

In the existing CLI command, the Load function of config.go is used, and the Load function checks for the existence of the config path and config.json file and creates one if it doesn't exist.

viper.ReadInConfig() primarily reads the configuration values based on the configname, configtype, and config path set in commands.go.

During this process, for users who have installed the yorkie program for the first time, the config.json does not exist in the ~/.yorkie directory. Therefore, if simply call viper.ReadInConfig(), an error occurs.

To resolve this, I utilized the logic of the Load function from config.go. Do you think there's a need to modify the logic, or would it be better to remove the duplicated logic from the Load method?

(Additionally, the statement path.Join(os.Getenv("HOME"), ".yorkie") in commands.go seems not to properly find the HOME directory in a Windows environment. Should this also be taken into consideration?)

Copy link
Member

Choose a reason for hiding this comment

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

Do you think there's a need to modify the logic, or would it be better to remove the duplicated logic from the Load method?

It will be better to refactor duplicate codes in Load() and ReadConfig().

Also, since ReadConfig()name is confusing with Load(), how about changing this function name to something like Preload()?

(Additionally, the statement path.Join(os.Getenv("HOME"), ".yorkie") in commands.go seems not to properly find the HOME directory in a Windows environment. Should this also be taken into consideration?)

We need to cover Windows as well. We might need to code like this: https://gist.github.com/miguelmota/f30a04a6d64bd52d7ab59ea8d95e54da

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the feedback. I will make the changes.

Copy link
Member

@hackerwins hackerwins left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution. CLI would be more convenient with context commands. I left a few comments.

A. Cannot logged in to api.yorkie.dev:443

I was not able to log in to api.yorkie.dev:443 with CLI from this PR. Could you find the reason of this?

$ yorkie login -u [ID] -p [PASSWORD] --rpc-addr api.yorkie.dev:443
Error: rpc error: code = Unavailable desc = connection error: desc = "error reading server preface: http2: frame too large"

B. Look and Feel of yorkie context ls command

Currently, CLI displays lists without borders, but yorkie context ls command displays list with borders. Could we remove the border from the command to keep UI consistency?

$ yorkie document ls codepair --rpc-addr api.yorkie.dev:443

 ID                        KEY                   CREATED AT  ACCESSED AT  UPDATED AT  SNAPSHOT
 64fe0ef0763112ce41c841d3  codepairs-cxz35r      32 hours    32 hours     32 hours    {"content":[],"shapes":[]}
 64fc0744763112ce41b34eb1  codepairs-wfkkzq      2 days      2 days       2 days      {"content":[{"val":"#"},{"val":" "},{"val":"H"},{"...

C. Display current context in yorkie context ls

It would be useful to display current context in yorkie context ls. We can refer to kubectl config get-contexts command.

$ kubectl config get-contexts
CURRENT   NAME                                                                  CLUSTER                                                               AUTHINFO                                                              NAMESPACE
          arn:aws:eks:ap-northeast-2:000000000000:cluster/yorkie-eks-XXXXXXXX   arn:aws:eks:ap-northeast-2:000000000000:cluster/yorkie-eks-XXXXXXXX   arn:aws:eks:ap-northeast-2:000000000000:cluster/yorkie-eks-XXXXXXXX
*         arn:aws:eks:ap-northeast-2:000000000000:cluster/yorkie-eks-YYYYYYYY   arn:aws:eks:ap-northeast-2:000000000000:cluster/yorkie-eks-YYYYYYYY   arn:aws:eks:ap-northeast-2:000000000000:cluster/yorkie-eks-YYYYYYYY
          minikube                                                              minikube                                                              minikube                                                              default

@Wu22e
Copy link
Contributor Author

Wu22e commented Sep 12, 2023

@hackerwins
Thank you for the feedback.

A. Cannot log in to api.yorkie.dev:443

I have not encountered this error before. I will try to reproduce the problem state and identify the problem and its solution. If you have any insights or possible causes, I would appreciate it.

B. Look and Feel of yorkie context ls command
C. Display current context in yorkie context ls

I have fully understood the intent of the feedback for B and C. I will apply this into the pull request.

@hackerwins
Copy link
Member

@Wu22e Thanks for your reply.

A. Cannot log in to api.yorkie.dev:443

I have not encountered this error before. I will try to reproduce the problem state and identify the problem and its solution. If you have any insights or possible causes, I would appreciate it.

I'm not sure why, but the error did not occur. I'll let you know if it occurs again.

@hackerwins hackerwins mentioned this pull request Oct 1, 2023
2 tasks
@hackerwins hackerwins closed this Oct 1, 2023
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.

Simplify CLI commands by saving rpc-addr and project to context
4 participants