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

Neither ReloadToSnapshot nor SnapshotBackup#restore work in domain mode #160

Closed
zebrik opened this issue May 24, 2017 · 7 comments
Closed
Assignees
Milestone

Comments

@zebrik
Copy link
Contributor

zebrik commented May 24, 2017

This is caused by server-config parameter being passed to the reload command instead of either domain-config or host-config.

@Ladicek
Copy link
Contributor

Ladicek commented May 24, 2017

This is actually very simple: I just don't know enough about domain. If you look at ReloadToOriginal, I made a bunch of comments to explain this -- I wasn't careful enough to do the same in ReloadToSnapshot / SnapshotBackup. And I think there was someone who worked with me on this who also didn't know much about domain :-)

I agree this is important, and I need to ask you for help. How do you think this should work?

@zebrik
Copy link
Contributor Author

zebrik commented May 24, 2017

The scope of this issue was originally to fix at least snapshots of domain.xml which is IMHO the main use case. To fix just this would be probably easy and I could create PR for this if you want.
The scope of #161 was to consider if we want to extend the SnapshotBackup functionality to enable backuping/restoring of host.xml as well. This is probably not so common requirement and would require more work.
Are there any domain tests in Creaper?

@Ladicek
Copy link
Contributor

Ladicek commented May 24, 2017

OK, I see. So fixing the backup/restore of domain.xml config would be just a simple change to ReloadFromSnapshot, right?

And the ability to backup/restore host.xml config would require more work both in SnapshotBackup and ReloadToSnapshot, right?

If so, I'm going to reopen #161 and if you provide a PR for this, that would be awesome!

@zebrik
Copy link
Contributor Author

zebrik commented May 24, 2017

Exactly. I can provide PR for #160 but without test since there are no module for domain tests so far - am I right?

@Ladicek
Copy link
Contributor

Ladicek commented May 24, 2017

I always wanted to have a domain testsuite, but never had the courage to actually implement it. So yes, you're right, there's no domain testsuite. YET! :-)

@zebrik
Copy link
Contributor Author

zebrik commented May 24, 2017

PR: #162

@Ladicek Ladicek assigned Ladicek and zebrik and unassigned Ladicek May 25, 2017
@Ladicek Ladicek added this to the 1.6.1 milestone May 25, 2017
@Ladicek
Copy link
Contributor

Ladicek commented May 25, 2017

Fixed in #162.

@Ladicek Ladicek closed this as completed May 25, 2017
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

No branches or pull requests

2 participants