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

New device and boot selection dialogs #1106

Merged
merged 14 commits into from Apr 3, 2024

Conversation

dgdavid
Copy link
Contributor

@dgdavid dgdavid commented Mar 19, 2024

This PR implements the new UI selectors for the target installation device and for the boot device, see https://github.com/openSUSE/agama/blob/master/doc/storage_ui.md.

For the installation device it offers the following options:

  • Select a target disk (the file systems are created over new partitions).
  • Create a new LVM volume group (the file systems are crated over logical volumes).
  • The option for reusing LVM volume groups is not offered yet.

For the boot device it offers the following options:

  • To use the selected disk for installation.
  • To use a different disk.
  • To not configure boot partitions.

Screenshots

Toggle

localhost_8080_ (33)

localhost_8080_ (32)

@coveralls
Copy link

coveralls commented Mar 25, 2024

Coverage Status

coverage: 75.12% (+0.03%) from 75.095%
when pulling 5e9c002 on storage-next-selector
into 7e1476c on storage-next-device-selection.

* @param {string[]} [props.initialExpandedKeys=[]] - Ids of initially expanded items.
* @param {(selection: Array<object>) => void} [props.onSelectionChange=noop] - Callback to be triggered when selection changes.
* @param {object} [props.tableProps] - Props for {@link https://www.patternfly.org/components/table/#table PF/Table}.
*/
Copy link
Contributor Author

@dgdavid dgdavid Mar 25, 2024

Choose a reason for hiding this comment

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

// TODO:

Add a note why this cannot be build with core/TreeTable.

Try to unify them as much as possible (after current milestone)

@dgdavid

This comment was marked as resolved.

@dgdavid

This comment was marked as resolved.

@joseivanlopez

This comment was marked as resolved.

Comment on lines 124 to 125
{_("Additional partitions to boot the system will be configured at /dev/vdc, \
the device selected for installing the system.")}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, the idea here is to go for a %s and then replace it by the selected installationDevice

Suggested change
{_("Additional partitions to boot the system will be configured at /dev/vdc, \
the device selected for installing the system.")}
{_("Additional partitions to boot the system will be configured in \
the device selected for installing the system, currently %s")}

But I'm not sure how to make it work. I mean, when target is disk is pretty clear, just to put the device.name or deviceLabel there. But what is the right value when the target is a new LVM thing (see https://github.com/openSUSE/agama/pull/1106/files#diff-bd6c15ca365f9adcb6bb3d76300f7729cfbd21274b895e1504c5956443e8a99cR53-R61)

Let's talk about it on Monday.


return (
<Text>
<span dangerouslySetInnerHTML={{ __html: msg1 }} />
Copy link
Contributor Author

Choose a reason for hiding this comment

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

NOTE for my self: I feel we're abusing of this technique. Try to looks for a better, different, or safer approach (if any).

@joseivanlopez joseivanlopez marked this pull request as ready for review April 3, 2024 12:01
@joseivanlopez joseivanlopez changed the title [WIP]: web: Change the storage device selection dialog New device and boot selection dialogs Apr 3, 2024
Copy link
Member

@ancorgs ancorgs left a comment

Choose a reason for hiding this comment

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

Content-wise it LGTM, but I guess we need to squash those 49 WIP commits(!) into something more reasonable.

joseivanlopez and others added 12 commits April 3, 2024 14:48
Co-authored-by: Ancor Gonzalez Sosa <ancorgs@gmail.com>
Co-authored-by: Ancor Gonzalez Sosa <ancorgs@gmail.com>
Co-authored-by: Ancor Gonzalez Sosa <ancorgs@gmail.com>
Co-authored-by: Ancor Gonzalez Sosa <ancorgs@gmail.com>
Co-authored-by: David Díaz González <dgonzalez@suse.de>
Co-authored-by: David Díaz González <dgonzalez@suse.de>
Co-authored-by: David Díaz González <dgonzalez@suse.de>
Co-authored-by: Ancor Gonzalez Sosa <ancorgs@gmail.com>
Co-authored-by: David Díaz González <dgonzalez@suse.de>
joseivanlopez and others added 2 commits April 3, 2024 15:33
Co-authored-by: Ancor Gonzalez Sosa <ancorgs@gmail.com>
Co-authored-by: David Díaz González <dgonzalez@suse.de>
Co-authored-by: Ancor Gonzalez Sosa <ancorgs@gmail.com>
Copy link
Member

@ancorgs ancorgs 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 re-checked everything, but the list of commits looks reasonable now.

So LGTM.

@joseivanlopez joseivanlopez merged commit 55ff0ba into storage-next-device-selection Apr 3, 2024
8 checks passed
@joseivanlopez joseivanlopez deleted the storage-next-selector branch April 3, 2024 15:07
joseivanlopez added a commit that referenced this pull request Apr 4, 2024
Merging feature branch which includes:

* #1068
* #1106
@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.

None yet

4 participants