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

Better AutoYaST import support #1244

Merged
merged 19 commits into from
May 24, 2024
Merged

Better AutoYaST import support #1244

merged 19 commits into from
May 24, 2024

Conversation

imobachgs
Copy link
Contributor

@imobachgs imobachgs commented May 21, 2024

  • Extend the converter to read some missing sections: networking, language, keyboard, timezone, and suse_register.
  • Document how supported sections are handled, including what is missing.
  • Improve test coverage.

Tasks

  • Read and document the networking section
  • Read and document the language section
  • Read and document the keyboard section
  • Read and document the timezone section
  • Read and document the suse_register section

@imobachgs imobachgs changed the title AutoYaST import support Better AutoYaST import support May 21, 2024
@imobachgs imobachgs force-pushed the autoyast-import branch 3 times, most recently from da25e4a to b060422 Compare May 21, 2024 14:08
@coveralls
Copy link

coveralls commented May 21, 2024

Coverage Status

coverage: 70.362% (+0.5%) from 69.873%
when pulling c324d4c on autoyast-import
into 6443351 on master.

@imobachgs imobachgs marked this pull request as ready for review May 23, 2024 23:39
@imobachgs imobachgs requested a review from jreidinger May 23, 2024 23:39
doc/autoyast.md Outdated Show resolved Hide resolved
.merge(software.read)
.merge(product.read)
.merge(storage.read)
.merge(l10n.read)
Copy link
Contributor

@jreidinger jreidinger May 24, 2024

Choose a reason for hiding this comment

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

NP, just matter of taste, but I would write it in more easier way for extend like:

READERS = ["user", "root", "software", ... ] # extend list to add support for more sections
def export_profile(profile)
  READERS.each_with_object({}) do |section, result|
    require "agama/autoyast/#{section}_reader}"
    klass = "#{section}_reader".split("_").map(&:capitalize).join
    reader = Agama::AutoYaST.get_const(klass).new(profile)
    result.merge(reader.read)
   end
end

this way it is really just single line to extend it with new section as opposite to current code which require change in three places.

Copy link
Contributor

Choose a reason for hiding this comment

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

btw I can even reduce it to no line in converter if we have something like agama/autoyast file that contain all classes and then filter it by known readers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point! I decided to go with the READERS variable approach because it is more explicit.

@imobachgs imobachgs merged commit abadf41 into master May 24, 2024
6 checks passed
@imobachgs imobachgs deleted the autoyast-import branch May 24, 2024 10:03
@imobachgs imobachgs mentioned this pull request Jun 27, 2024
imobachgs added a commit that referenced this pull request Jun 27, 2024
Prepare for releasing Agama 9. It includes the following pull requests:

- #1101
- #1202
- #1228
- #1231
- #1236
- #1238
- #1239
- #1240
- #1242
- #1243
- #1244
- #1245
- #1246
- #1247
- #1248
- #1249
- #1250
- #1251
- #1252
- #1253
- #1254
- #1255
- #1256
- #1257
- #1258
- #1259
- #1260
- #1261
- #1264
- #1265
- #1267
- #1268
- #1269
- #1270
- #1271
- #1272
- #1273
- #1274
- #1279
- #1280
- #1284
- #1285
- #1286
- #1287
- #1288
- #1289
- #1290
- #1291
- #1292
- #1293
- #1294
- #1295
- #1296
- #1298
- #1299
- #1300
- #1301
- #1302
- #1303
- #1304
- #1305
- #1306
- #1307
- #1308
- #1309
- #1310
- #1311
- #1312
- #1313
- #1314
- #1315
- #1316
- #1317
- #1318
- #1319
- #1320
- #1321
- #1322
- #1323
- #1324
- #1325
- #1326
- #1328
- #1329
- #1331
- #1332
- #1334
- #1338
- #1340
- #1341
- #1342
- #1343
- #1344
- #1345
- #1348
- #1349
- #1351
- #1352
- #1353
- #1354
- #1355
- #1356
- #1357
- #1358
- #1359
- #1360
- #1361
- #1362
- #1363
- #1365
- #1366
- #1367
- #1368
- #1371
- #1372
- #1374
- #1375
- #1376
- #1379
- #1380
- #1381
- #1383
- #1384
- #1385
- #1386
- #1387
- #1388
- #1389
- #1391
- #1392
- #1394
- #1395
- #1397
- #1398
- #1399
- #1400
- #1403
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