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

fix(service): Copy the libzypp caches and credentials to the installed system #1357

Merged
merged 4 commits into from
Jun 19, 2024
Merged

fix(service): Copy the libzypp caches and credentials to the installed system #1357

merged 4 commits into from
Jun 19, 2024

Conversation

lslezak
Copy link
Contributor

@lslezak lslezak commented Jun 19, 2024

Problem

  • The libzypp caches and credentials are not copied to the installed system
  • Installing any package or even simple zypper refresh triggers full repository reload

Solution

  • Just copy the files from the Live ISO zypp repository to the installed system

Testing

  • Added a new unit test
  • Tested manually, after installation all caches are copied and zypper refresh does nothing

@coveralls
Copy link

Coverage Status

coverage: 70.921% (+0.04%) from 70.877%
when pulling ce28b91 on copy_zypp_cache
into 043f87d on master.

imobachgs
imobachgs reviewed Jun 19, 2024
View reviewed changes
service/test/agama/software/manager_test.rb Outdated
allow(Dir).to receive(:exist?).and_call_original

# copying the raw cache
expect(Dir).to receive(:exist?).with("/run/agama/zypp/var/cache/zypp/raw").and_return(true)
Copy link
Contributor

Choose a reason for hiding this comment

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

np: Although we might consider it good enough, this test just makes sure that you call the expected methods. What about a test that actually performs the copy and checks the result?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To be honest I do not like unit tests which really touch the system. For integration tests it is OK but for unit tests I'd rather avoid that if not necessary. And it actually means you are testing the Ruby functionality, not our code.

Copy link
Contributor

Choose a reason for hiding this comment

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

I can understand that, but I would say that we are not writing unit tests "by the book" and abusing mocks can produce tests that are not that useful after all. Actually, that test knows a lot about implementation details, which looks wrong to me.

However, as said, I will not block this PR just because of that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see your point. In such case the test just tests the mocks themselves.

But if we test the real result then we are actually testing the external code and the test is more fragile. If the code basically only calls external code and nothing else then I think it is acceptable.

@coveralls
Copy link

Coverage Status

coverage: 70.921% (+0.04%) from 70.877%
when pulling 32946d5 on copy_zypp_cache
into 043f87d on master.

mvidner
mvidner reviewed Jun 19, 2024
View reviewed changes
service/package/rubygem-agama-yast.changes
system package management (gh#openSUSE/agama#1329)
- Properly delete the libzypp cache when changing the products
(gh#openSUSE/agama#1349)

Copy link
Contributor

Choose a reason for hiding this comment

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

OK so shouldn't there be an entry for this PR too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, it is actually a fix up for #1329 so I do not think it needs any special entry.

@lslezak lslezak merged commit 3a77a14 into master Jun 19, 2024
6 checks passed
@lslezak lslezak deleted the copy_zypp_cache branch June 19, 2024 12:42
@imobachgs imobachgs added this to the Agama 9 milestone Jun 25, 2024
@imobachgs imobachgs mentioned this pull request Jun 27, 2024
Merged
imobachgs added a commit that referenced this pull request Jun 27, 2024
@imobachgs
Update to release 9 (#1404)
8fe19db 
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
Reviewers

@mvidner mvidner mvidner left review comments

@imobachgs imobachgs imobachgs approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
Agama 9
Development

Successfully merging this pull request may close these issues.
4 participants
@lslezak @coveralls @imobachgs @mvidner