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

Adapted the CLI consuming the HTTP/JSON network API #1178

Merged
merged 11 commits into from
May 8, 2024
Merged

Adapted the CLI consuming the HTTP/JSON network API #1178

merged 11 commits into from
May 8, 2024

Conversation

teclator
Copy link
Contributor

@teclator teclator commented May 3, 2024 •
edited
Loading

Problem

Ideally, the CLI and the web UI should use the same interface to interact with Agama. As it is a time that may take some time lets start updating the network configuration as it stopped to work once we moved to the HTTP/JSON API.

Solution

Adapted the CLI using the HTTP/JSON API. For authentication the stored JWT token or the agama-live token will be used when exist. Cookires are enabled just in case in the future we wanted to save them to a file and restoring when the client is obtained.

Last but not least, rpassword was added for hidding the password when it is entered for authentication.

Testing

  • Added a new unit test
  • Tested manually

@teclator teclator changed the title Network cli Adapted the CLI consuming the http/JSON network API May 5, 2024
@coveralls
Copy link

coveralls commented May 6, 2024 •
edited
Loading

Coverage Status

coverage: 65.81% (-2.3%) from 68.078%
when pulling 47952af on network_cli
into 259b625 on architecture_2024.

@teclator teclator marked this pull request as ready for review May 7, 2024 07:33
@teclator teclator requested a review from imobachgs May 7, 2024 07:33
@imobachgs imobachgs changed the title Adapted the CLI consuming the http/JSON network API Adapted the CLI consuming the HTTP/JSON network API May 7, 2024
imobachgs
imobachgs requested changes May 7, 2024
View reviewed changes
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 have not finished with the review yet, just a partial one.

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/config.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
rust/agama-cli/src/config.rs Outdated Show resolved Hide resolved
rust/agama-cli/src/config.rs Outdated Show resolved Hide resolved
imobachgs
imobachgs requested changes May 7, 2024
View reviewed changes
rust/agama-cli/src/auth.rs
@@ -109,6 +121,10 @@ impl Credentials for MissingCredentials {
fn jwt_file() -> Option<PathBuf> {
Some(home::home_dir()?.join(DEFAULT_JWT_FILE))
}
/// Path to agama-live token file.
fn agama_token_file() -> Option<PathBuf> {
home::home_dir().map(|p| p.join(DEFAULT_AGAMA_TOKEN_FILE))
Copy link
Contributor

Choose a reason for hiding this comment

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

There is nothing like a default Agama token file. Actually, the serve token allows to pass a path through the --generate-token option.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we could ignore it then, I just followed the PBI suggestions which mentioned the /run/agama/token file

Copy link
Contributor

Choose a reason for hiding this comment

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

For the live media, I guess we will inject that token into root's home directory. Or we can decide that we are going to use always that file (we could even have a AGAMA_RUN_DIR environment variable).

rust/agama-lib/src/error.rs Show resolved Hide resolved
rust/agama-lib/src/lib.rs Outdated Show resolved Hide resolved
rust/agama-lib/src/lib.rs Outdated Show resolved Hide resolved
rust/agama-lib/src/network/store.rs Show resolved Hide resolved
rust/agama-lib/src/store.rs Outdated Show resolved Hide resolved
rust/agama-lib/src/lib.rs Show resolved Hide resolved
rust/agama-server/src/network/web.rs Outdated Show resolved Hide resolved
@teclator teclator requested a review from imobachgs May 7, 2024 14:45
imobachgs
imobachgs reviewed May 7, 2024
View reviewed changes
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.

In general it looks rather good. We just need to agree on how to handle the token path.

@teclator
Copy link
Contributor Author

teclator commented May 8, 2024

Adapted

@teclator teclator requested a review from imobachgs May 8, 2024 09:58
imobachgs
imobachgs requested changes May 8, 2024
View reviewed changes
Copy link
Contributor

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

Oh, I forgot about something: did you remove the --generate-token option from the .service file? I mean this line.

@teclator
Copy link
Contributor Author

teclator commented May 8, 2024

Oh, I forgot about something: did you remove the --generate-token option from the .service file? I mean this line.

Good catch ;), removed

@teclator teclator merged commit 4641c2e into architecture_2024 May 8, 2024
1 check passed
@teclator teclator deleted the network_cli branch May 8, 2024 13:52
@imobachgs imobachgs mentioned this pull request May 17, 2024
Merged
imobachgs added a commit that referenced this pull request May 17, 2024
@imobachgs
Update to release 8 (#1232)
b13e7b7 
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
Reviewers

@imobachgs imobachgs imobachgs 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.
3 participants
@teclator @coveralls @imobachgs