-
Notifications
You must be signed in to change notification settings - Fork 42
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
initial global btrfs option #1039
Conversation
1a. If you refer to the table, don't put too much work on it, we need to rework it anyway.
|
We already have a short explanation in the filesystem selector: "Allows rolling back any change done to the system and restoring its previous state", see https://github.com/openSUSE/agama/blob/master/web/src/components/storage/VolumeForm.jsx#L212. Maybe we could reuse it. |
This comment was marked as outdated.
This comment was marked as outdated.
About the translation from settings to UI that was being discussed on IRC:
So: If If Last but not least, if
As a cherry on top, during a recent conversation with @dgdavid we concluded the explanation could be extended a bit to "Uses btrfs for the root file system allowing to boot to a previous version of the system after configuration changes or software upgrades". |
const initialChecked = rootVolume !== undefined && rootVolume.fsType === "Btrfs" && rootVolume.snapshots; | ||
const [isChecked, setIsChecked] = useState(initialChecked); | ||
|
||
if (rootVolume === undefined) { // no root volume is probably some error or still loading |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NP: I think it is better putting the comment in the line above
} else { // strange situation, should not happen | ||
return (<></>); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since React 18, this is no longer needed.
Now React allows returning undefined
, and this is the default return value from a JavaScript function when you returns nothing. See https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/function#description.
d2fa735
to
3aa43e0
Compare
This comment was marked as resolved.
This comment was marked as resolved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general changes looks good. Just a few minor comments.
Regard to screenshots in the PR description, as agreed in IRC we can go ahead without updating them to the latest changes introduced in #1045.
}) => { | ||
const rootVolume = (settings.volumes || []).find((i) => i.mountPath === "/"); | ||
|
||
const initialChecked = rootVolume !== undefined && rootVolume.fsType === "Btrfs" && rootVolume.snapshots; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not for now, but I think we should go for something similar to https://github.com/openSUSE/agama/blob/76c930c06e1ea4bb24fa11c023cd9a94e09337fd/web/src/client/storage.js#L55-L58 for having the Fstypes in a "constant".
const forcedSnapshots = !configurableSnapshots && rootVolume.fsType === "Btrfs" && rootVolume.snapshots; | ||
|
||
const SnapshotsToggle = () => { | ||
const explanation = _("Uses btrfs for the root file system allowing to boot to a previous version of the system after configuration changes or software upgrades."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here I would change two things:
- btrfs -> Btrfs
- Use the
<abbr>
wrappingBtrfs
. you can see examples at https://github.com/openSUSE/agama/blob/76c930c06e1ea4bb24fa11c023cd9a94e09337fd/web/src/components/storage/ProposalDeviceSection.jsx#L409-L418 and https://github.com/openSUSE/agama/blob/76c930c06e1ea4bb24fa11c023cd9a94e09337fd/web/src/components/storage/ProposalSettingsSection.jsx#L80-L89
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking it twice, the second point can be postponed for another PR in which we should extract that Description
component wrapping the span with dangerousSetInnerHTML attribute. Using a meaningful name, of course.
@ancorgs, ^^^
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeap, having in translatable string html tags is definitively something nice....changing name will happen in few minutes
@@ -237,11 +296,28 @@ export default function ProposalSettingsSection({ | |||
onChange({ encryptionPassword: password, encryptionMethod: method }); | |||
}; | |||
|
|||
const changeBtrfsSnapshots = ({ value, settings }) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NP: I'd use active
instead of value
.
Also, do not forget to add an entry in the .changes file |
Co-authored-by: David Díaz <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Co-authored-by: David Díaz <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
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
Problem
Follow up of:
Right now Btrfs snapshots are shown in the UI as an special kind of filesystem type for the root volume in the table of file-systems.
But later we realized is something more relevant because it affects what you can do regarding booting (no separate
/boot
partition, for example), the volume sizes or the approach to btrfs subvolumes.Solution
Make btrfs with snapshots as global option that modify how root volume looks like.
Testing
Screenshots
How it looks for product that require btrfs with snapshots
Edit form for root when btrfs is not with snapshots
Edit form for root when btrfs is with snapshots
Global option for btrfs with snapshots