Skip to content

Add support to expose the Issues API#1100

Merged
imobachgs merged 9 commits intoarchitecture_2024from
http-issues-api
Mar 20, 2024
Merged

Add support to expose the Issues API#1100
imobachgs merged 9 commits intoarchitecture_2024from
http-issues-api

Conversation

@imobachgs
Copy link
Contributor

@imobachgs imobachgs commented Mar 18, 2024

Trello: https://trello.com/c/7SUGZlbi/349-expose-the-issues-interface-over-http

  • It enables the org.opensuse.Agama1.Issue interface over D-Bus.
  • It fixes a problem when handling status changes (WithStatus).

@coveralls
Copy link

Coverage Status

coverage: 74.412% (-0.09%) from 74.498%
when pulling 3e6ae2a on http-issues-api
into 80ac668 on better-errors.

Base automatically changed from better-errors to architecture_2024 March 18, 2024 20:58
@imobachgs imobachgs marked this pull request as ready for review March 19, 2024 13:52
.merge(status_router)
.merge(progress_router)
.nest("/issues/product", product_issues)
.nest("/issues/software", software_issues)
Copy link
Contributor

Choose a reason for hiding this comment

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

to be honest I am not sure if I expect subtree under issue or other way /software/issues. Probably later makes more sense to me. Or having general /issues call. Would it be possible?

.await?,
);
stream.insert(
"software-product-issues",
Copy link
Contributor

Choose a reason for hiding this comment

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

similar here. I would either expect some id in issue report that can be used to make it specific or at least use just product-issues, otherwise naming looks inconsistent

Copy link
Contributor Author

@imobachgs imobachgs Mar 20, 2024

Choose a reason for hiding this comment

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

I am not using those IDs for anything, so they aren't relevant. I decided to, somehow, match the D-Bus structure (/org/opensuse/Agama/Software1 and /org/opensuse/Agama/Software1/Product).

However, if we use it as an ID, we could get rid of the service, hiding to the client the fact that it is talking to a D-Bus service:

WithIssues("software/issues")

instead of

WithIssues("software/issues", "/org/opensuse/Agama/Software1"

In that case, I would adjust the events-related types to something like:

pub type Scope = String;
pub type EventsSender = Sender<(Scope, Event)>;
pub type EventsReceiver = Receiver<(Scope, Event)>;

Does it make sense?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On the other hand, keeping the scope out of the event introduces some small inconsistencies, like being able to send a LocaleChanged event from any unrelated scope. But I guess I can live with that.

I have been also thinking about using something like:

pub enum Event {
  Locale(LocaleEvent),
  Software(SoftwareEvent),
};

And then each service defines its event types:

pub enum LocaleEvent {
  LocaleChanged { locale: String },
  IssuesChanged(IssuesChangedEvent),
  ProgressChanged(ProgressChangedEvent)
}

In this case, the scope is implicit.

I would like to think a little bit about that (and other options we could think of). For now, I will just update the IDs to make them look better and more consistent.

Copy link
Contributor

Choose a reason for hiding this comment

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

makes sense to me to think a bit about it and what is more important document decision, so we will be consistent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just a note about the second proposal: if you can emit a IssuesChange in several places from a single service, you still need a way to differentiate them.

return this.client.onEvent("Progress", (progress) => {
if (progress?.service === service_name) {
return this.client.onEvent("Progress", ({ service, ...progress }) => {
if (service === service_name) {
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@imobachgs imobachgs merged commit 0cc18ac into architecture_2024 Mar 20, 2024
@imobachgs imobachgs deleted the http-issues-api branch March 20, 2024 07:14
imobachgs added a commit that referenced this pull request May 6, 2024
After a few months of work, it is time to merge the `architecture_2024`
branch into `master`. It is still a work-in-progress, but all the
efforts should be go now against that branch.

## Pull requests

* #1061
* #1064
* #1073
* #1074
* #1080
* #1089
* #1091
* #1092
* #1094
* #1095
* #1099
* #1100
* #1102
* #1103
* #1112
* #1114
* #1116
* #1117
* #1119
* #1120
* #1123
* #1126
* #1129
* #1130
* #1131
* #1132
* #1133
* #1134
* #1136
* #1139
* #1140
* #1143
* #1146

## Other commits

* 8efa41f
* 9e2dec0
@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