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

Implementation of Server {Login|Logout} commands #1067

Merged
merged 27 commits into from
Mar 7, 2024

Conversation

mchf
Copy link
Contributor

@mchf mchf commented Mar 4, 2024

Problem

We need authorization token to be able to proceed with subsequent commands

Solution

agama server login asks for JWT and stores it into a well-known file
agama server logout deletes stored JWT

Testing

  • Tested manually

@mchf mchf requested a review from imobachgs March 4, 2024 16:12
@coveralls
Copy link

coveralls commented Mar 4, 2024

Coverage Status

coverage: 74.141% (-0.3%) from 74.401%
when pulling 4754ad9 on agama-cli_login_subcommand
into 987e8f7 on master.

@mchf mchf force-pushed the agama-cli_login_subcommand branch from 205f47a to f41d88a Compare March 5, 2024 07:15
Copy link
Contributor

@imobachgs imobachgs left a comment

Choose a reason for hiding this comment

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

I made a few comments (nothing critical), although I would probably only allow reading the password from stdin or, at most, from an environment variable. Of course, as a fallback, I would ask interactively (as you did). I guess it would be more flexible and simpler to implement :-)

rust/agama-cli/src/commands.rs Outdated Show resolved Hide resolved
rust/agama-cli/src/server.rs Outdated Show resolved Hide resolved
rust/agama-cli/src/server.rs Outdated Show resolved Hide resolved
rust/agama-cli/src/server.rs Outdated Show resolved Hide resolved
rust/agama-cli/src/server.rs Outdated Show resolved Hide resolved
rust/agama-cli/src/server.rs Outdated Show resolved Hide resolved
service/Gemfile.lock Outdated Show resolved Hide resolved
rust/agama-cli/src/server.rs Outdated Show resolved Hide resolved
rust/agama-cli/src/server.rs Outdated Show resolved Hide resolved
@mchf mchf force-pushed the agama-cli_login_subcommand branch 4 times, most recently from 5594006 to b7c7495 Compare March 6, 2024 11:27
@mchf mchf requested a review from imobachgs March 6, 2024 19:12
@mchf mchf force-pushed the agama-cli_login_subcommand branch 2 times, most recently from 47438ee to 6ef471d Compare March 6, 2024 21:22
rust/agama-cli/src/auth.rs Outdated Show resolved Hide resolved

#[derive(Subcommand, Debug)]
pub enum AuthCommands {
/// Login with defined server. Result is JWT stored locally and made available to
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure about the help text. Perhaps @mvidner could have a look.

}

/// Reads stored JWT token and returns it
pub fn jwt() -> anyhow::Result<String> {
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to be public.

Suggested change
pub fn jwt() -> anyhow::Result<String> {
fn jwt() -> anyhow::Result<String> {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i made it public to make it available for possible further use throughout the agama, if you don't think it will ever happen i'll remove it.

Copy link
Contributor

Choose a reason for hiding this comment

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

We can make it public when we need it to be public 😉. If we want to make it reusable, perhaps it should be in agama-lib.

rust/agama-cli/src/auth.rs Outdated Show resolved Hide resolved
rust/agama-cli/src/auth.rs Show resolved Hide resolved
rust/agama-cli/src/auth.rs Outdated Show resolved Hide resolved
rust/agama-cli/src/auth.rs Outdated Show resolved Hide resolved
rust/agama-cli/src/auth.rs Outdated Show resolved Hide resolved
@mchf mchf force-pushed the agama-cli_login_subcommand branch from 6ef471d to 9042a19 Compare March 7, 2024 09:50
@mchf mchf requested a review from imobachgs March 7, 2024 09:51
Copy link
Contributor

@imobachgs imobachgs left a comment

Choose a reason for hiding this comment

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

It looks good overall (although I would like to have the text reviewed by @mvidner). But that's something we can do when we review the CLI's overall help, which is far from ideal.

As a note, I really think we should read stuff from stdin, but this PR implements what it was in the PBI. So I will create another one for the future.

Last but not least, please, do not "force-push" during a review. It makes it harder to review the changes (for instance, the Compare link from GitHub might be useless then).

rust/agama-cli/src/auth.rs Outdated Show resolved Hide resolved
@mchf mchf marked this pull request as ready for review March 7, 2024 10:50
@mchf mchf merged commit c43827a into master Mar 7, 2024
1 check passed
@mchf mchf deleted the agama-cli_login_subcommand branch March 7, 2024 11:09
@imobachgs imobachgs mentioned this pull request May 17, 2024
imobachgs added a commit that referenced this pull request May 17, 2024
Prepare for releasing Agama 8. It includes the following pull requests:

* #884
* #886
* #914
* #918
* #956
* #957
* #958
* #959
* #960
* #961
* #962
* #963
* #964
* #965
* #966
* #969
* #970
* #976
* #977
* #978
* #979
* #980
* #981
* #983
* #984
* #985
* #986
* #988
* #991
* #992
* #995
* #996
* #997
* #999
* #1003
* #1004
* #1006
* #1007
* #1008
* #1009
* #1010
* #1011
* #1012
* #1014
* #1015
* #1016
* #1017
* #1020
* #1022
* #1023
* #1024
* #1025
* #1027
* #1028
* #1029
* #1030
* #1031
* #1032
* #1033
* #1034
* #1035
* #1036
* #1038
* #1039
* #1041
* #1042
* #1043
* #1045
* #1046
* #1047
* #1048
* #1052
* #1054
* #1056
* #1057
* #1060
* #1061
* #1062
* #1063
* #1064
* #1066
* #1067
* #1068
* #1069
* #1071
* #1072
* #1073
* #1074
* #1075
* #1079
* #1080
* #1081
* #1082
* #1085
* #1086
* #1087
* #1088
* #1089
* #1090
* #1091
* #1092
* #1093
* #1094
* #1095
* #1096
* #1097
* #1098
* #1099
* #1100
* #1102
* #1103
* #1104
* #1105
* #1106
* #1109
* #1110
* #1111
* #1112
* #1114
* #1116
* #1117
* #1118
* #1119
* #1120
* #1121
* #1122
* #1123
* #1125
* #1126
* #1127
* #1128
* #1129
* #1130
* #1131
* #1132
* #1133
* #1134
* #1135
* #1136
* #1138
* #1139
* #1140
* #1141
* #1142
* #1143
* #1144
* #1145
* #1146
* #1147
* #1148
* #1149
* #1151
* #1152
* #1153
* #1154
* #1155
* #1156
* #1157
* #1158
* #1160
* #1161
* #1162
* #1163
* #1164
* #1165
* #1166
* #1167
* #1168
* #1169
* #1170
* #1171
* #1172
* #1173
* #1174
* #1175
* #1177
* #1178
* #1180
* #1181
* #1182
* #1183
* #1184
* #1185
* #1187
* #1188
* #1189
* #1190
* #1191
* #1192
* #1193
* #1194
* #1195
* #1196
* #1198
* #1199
* #1200
* #1201
* #1203
* #1204
* #1205
* #1206
* #1207
* #1208
* #1209
* #1210
* #1211
* #1212
* #1213
* #1214
* #1215
* #1216
* #1217
* #1219
* #1220
* #1221
* #1222
* #1223
* #1224
* #1225
* #1226
* #1227
* #1229
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.

3 participants