Store and restore original UCI configuration #76
Conversation
okraits
force-pushed
the
tdt-restore-unmodified-config
branch
from
96a5ea8 to
95d62c7
Compare
okraits
changed the title
okraits
force-pushed
the
tdt-restore-unmodified-config
branch
from
95d62c7 to
5805a91
Compare
|
@nemesisdesign How do you want to proceed with this one? |
nemesifier
added
enhancement
help wanted
important
There was a problem hiding this comment.
@okraits a basic test of this patch in the develop branch went well.
I cannot find time to write tests for this now and I am not sure who can do that, if you can't write tests for this we have to choose between leave the PR open until somebody can write the tests or merge without tests, both options are not optimal.
Do you think you can find the time to add a test for this case?
| utils.remove_uci_options(standard, file, section) | ||
| else | ||
| -- section is in the backup configuration -> restore | ||
| -- delete all options first |
There was a problem hiding this comment.
I'd move these two lines of comments before the else for better readability
There was a problem hiding this comment.
Done. I also moved the comment below the if above the if for consistency.
okraits
force-pushed
the
tdt-restore-unmodified-config
branch
from
5805a91 to
c2ea451
Compare
|
@nemesisdesign I can write tests, I just need to find time to do it. Maybe I will have some time for it in the next weeks. |
okraits
force-pushed
the
tdt-restore-unmodified-config
branch
2 times, most recently
from
f6628b1 to
f2e4df8
Compare
|
@okraits did you send an update to this PR in the last 8 hours? |
Just a rebase to master. I started working on a unittest for this but it will take some time to get all cases covered. |
|
@okraits great! |
okraits
force-pushed
the
tdt-restore-unmodified-config
branch
from
f2e4df8 to
3e1bf40
Compare
| if not utils.starts_with_dot(option) then | ||
| standard:delete(file, section['.name'], option) |
There was a problem hiding this comment.
Hi @nemesisdesign @okraits
Here, if we are going to write the section, then what's the point of removing options before
There was a problem hiding this comment.
The exact section from the backup configuration needs to be restored, so all existing options are removed so the restore can start from a clean state.
I think this is needed because utils.write_uci_section() doesn't remove a section before writing it: https://github.com/openwisp/openwisp-config/blob/master/openwisp-config/files/lib/openwisp/utils.lua#L66
We need to make sure that there are no old leftover options and that the section is restored as it was before openwisp-config modified it.
@devkapilbansal BTW great that you started to work on a test for this!
There was a problem hiding this comment.
The exact section from the backup configuration needs to be restored, so all existing options are removed so the restore can start from a clean state.
Thanks for clearing this 😄
I wrote some tests for this here, a1e565b. I tried but wasn't to able update this pull request.
There was a problem hiding this comment.
@okraits Can you please update this PR and check if tests are correct
I created a new branch on this repo 👇
https://github.com/openwisp/openwisp-config/tree/tdt-restore-unmodified-config
|
Superseded by #144 |
I think this patch for #72 is finished now. I tested it in different situations with openwisp-controller.
@nemesisdesign Please review when you have the time.
I think it makes sense that somebody else writes a unittest so that the topic is thought through by two different people.