Landscape Client

Merge lp://qastaging/~bogdana/landscape-client/save-aptsources into lp://qastaging/~landscape/landscape-client/trunk

  1. save-aptsources
  2. Merge into trunk
Proposed by Bogdana Vereha
Status: Merged
Approved by: Bogdana Vereha
Approved revision: 932
Merged at revision: 929
Proposed branch: lp://qastaging/~bogdana/landscape-client/save-aptsources
Merge into: lp://qastaging/~landscape/landscape-client/trunk
Diff against target: 251 lines (+124/-62)
2 files modified
landscape/manager/aptsources.py (+24/-23)
landscape/manager/tests/test_aptsources.py (+100/-39)
To merge this branch: bzr merge lp://qastaging/~bogdana/landscape-client/save-aptsources
Reviewer Review Type Date Requested Status
🤖 Landscape Builder test results Approve
Alberto Donato (community) Approve
Adam Collard (community) 🚴 Abstain
Данило Шеган (community) Approve
Review via email: mp+313608@code.qastaging.launchpad.net

Commit message

Save the original sources.list file when a repository profile is associated with a computer and restore it when the profile is removed.

Description of the change

Save the original sources.list file when a repository profile is associated with a computer and restore it when the profile is removed.

Note: this changed was based on the assumption that when a profile is removed or computer is disassociated, the server sends an apt-sources-replace message with an empty sources list (and that's the only case when this happens)

Testing instructions:
1. Follow the instructions in the linked bug to create a repository profile and associate a tag to it
2. Tag a computer that runs the updated version of the landscape client
3. Check that a sources.list.save file was created
4. Untag the computer
5. Check that the original contents of the sources.list file were restored

To post a comment you must log in.
Revision history for this message
🤖 Landscape Builder (landscape-builder) :
review: Abstain (executing tests)
Revision history for this message
🤖 Landscape Builder (landscape-builder) wrote :

Command: TRIAL_ARGS=-j4 make check
Result: Success
Revno: 925
Branch: lp:~bogdana/landscape-client/save-aptsources
Jenkins: https://ci.lscape.net/job/latch-test-precise/842/

review: Approve (test results)
lp://qastaging/~bogdana/landscape-client/save-aptsources updated
926. By Bogdana Vereha

Fix string formatting for file names

Revision history for this message
🤖 Landscape Builder (landscape-builder) :
review: Abstain (executing tests)
Revision history for this message
🤖 Landscape Builder (landscape-builder) wrote :

Command: TRIAL_ARGS=-j4 make check
Result: Success
Revno: 926
Branch: lp:~bogdana/landscape-client/save-aptsources
Jenkins: https://ci.lscape.net/job/latch-test-precise/843/

review: Approve (test results)
lp://qastaging/~bogdana/landscape-client/save-aptsources updated
927. By Bogdana Vereha

Revert last commit since it included build info

928. By Bogdana Vereha

Fix string formatting for file names

Revision history for this message
🤖 Landscape Builder (landscape-builder) :
review: Abstain (executing tests)
Revision history for this message
🤖 Landscape Builder (landscape-builder) wrote :

Command: TRIAL_ARGS=-j4 make check
Result: Success
Revno: 928
Branch: lp:~bogdana/landscape-client/save-aptsources
Jenkins: https://ci.lscape.net/job/latch-test-precise/844/

review: Approve (test results)
Revision history for this message
Данило Шеган (danilo) wrote :

This looks good as a fix, but I think you should drop now almost dead-code. I won't block on that (also because I get back only the second week of 2017).

A few of the suggestions I am not sure about: files as context managers would be much nicer, but not sure if landscape-client needs to support precise or whatever other Ubuntu release and what Python is there and if it supports using file objects as context managers (though https://docs.python.org/2/library/stdtypes.html#bltin-file-objects says that this is supported since 2.6, so I guess we are safe).

review: Approve
Revision history for this message
Adam Collard (adam-collard) wrote :

Did you consider saving the "old" sources list on the Landscape server instead? From the back of an envelope something like:

client C -> server S: here's my current sources.list
<time passes, repository profile used>
server S -> client C: use these sources.list (from profile)
<time passes, repository profile disabled>
server S -> client C: use these sources.list (last-known from client, before we switched its world over)

I feel a bit uneasy about using the client's filesystem as a data-store

review: Needs Information
Revision history for this message
Adam Collard (adam-collard) wrote :

I'm going to abstain to avoid blocking progress, please address comments though :)

review: Abstain (🚴)
Revision history for this message
Данило Шеган (danilo) wrote :

@Adam, ISTR we already use the client's filesystem to "store" disabled repos in /etc/apt/sources.list.d/*.list before setting up repository profiles using exactly the same approach (rename them to .save and that's it). I think making top-level sources.list behave the same is an improvement of the current state of things.

lp://qastaging/~bogdana/landscape-client/save-aptsources updated
929. By Bogdana Vereha

Merge from trunk

930. By Bogdana Vereha

Use files as context managers and replace content of sources.list

Revision history for this message
🤖 Landscape Builder (landscape-builder) :
review: Abstain (executing tests)
Revision history for this message
🤖 Landscape Builder (landscape-builder) wrote :

Command: TRIAL_ARGS=-j4 make check
Result: Success
Revno: 930
Branch: lp:~bogdana/landscape-client/save-aptsources
Jenkins: https://ci.lscape.net/job/latch-test-precise/847/

review: Approve (test results)
Revision history for this message
Bogdana Vereha (bogdana) wrote :

@Danilo, thanks for the suggestions, they should all be implemented now.
@Adam, while I agree that this solution isn't foolproof, I feel that making the server store the files would require a bigger change. And as Danilo said, since we've already been doing this for the files in sources.list.d, it might be worth investigating if any clients ever complained about it before making the improvement.

Revision history for this message
Alberto Donato (ack) wrote :

Looks good, +1!

Just a few nits inline.

review: Approve
lp://qastaging/~bogdana/landscape-client/save-aptsources updated
931. By Bogdana Vereha

Fix epydoc and remove unnecessary checks

Revision history for this message
🤖 Landscape Builder (landscape-builder) :
review: Abstain (executing tests)
Revision history for this message
🤖 Landscape Builder (landscape-builder) wrote :

Command: TRIAL_ARGS=-j4 make check
Result: Success
Revno: 931
Branch: lp:~bogdana/landscape-client/save-aptsources
Jenkins: https://ci.lscape.net/job/latch-test-precise/852/

review: Approve (test results)
lp://qastaging/~bogdana/landscape-client/save-aptsources updated
932. By Bogdana Vereha

Fix linting errors

Revision history for this message
🤖 Landscape Builder (landscape-builder) :
review: Abstain (executing tests)
Revision history for this message
🤖 Landscape Builder (landscape-builder) wrote :

Command: TRIAL_ARGS=-j4 make check
Result: Success
Revno: 932
Branch: lp:~bogdana/landscape-client/save-aptsources
Jenkins: https://ci.lscape.net/job/latch-test-precise/853/

review: Approve (test results)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches

to all changes: