Skip to content

Remove backup classes #1407

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

Conversation

ghoneycutt
Copy link
Contributor

Suggest that we remove these backup related classes. The work is useful and should be kept as example profiles. Backups are extremely site specific and having them in their current state is not providing value. These would do better converted to example profiles to show how you can do backups using these different methods. Moving them to examples also makes the module easier to maintain.

@ghoneycutt ghoneycutt requested a review from a team as a code owner June 29, 2021 01:48
@pmcmaw
Copy link
Contributor

pmcmaw commented Jul 5, 2021

We are currently planning work on doing a complete revamp of backups as soon as the team get a chance. See epic here: https://tickets.puppetlabs.com/browse/IAC-1661

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@ghoneycutt ghoneycutt changed the title WIP - Remove backup classes Remove backup classes Apr 26, 2022
@ghoneycutt
Copy link
Contributor Author

@LukasAud Could you please take a look at this one. It is similar in nature to the one you just merged.

@alexjfisher
Copy link
Collaborator

@LukasAud Could you please take a look at this one. It is similar in nature to the one you just merged.

Is it? mysql::server::backup is documented in the README, and is not related to unsupported Operating Systems. There's also #1447 open where the author has been asked to rebase so that the change can be reviewed properly before merging.

I'm against removing this functionality which I assume actually has a fair few users and seen willingness from community members to contribute fixes and enhancements.

@ghoneycutt
Copy link
Contributor Author

It is removing more cruft :) @alexjfisher Check out the ticket for more info. Basically backups are too specialized to the user's environment so an example would be better than actual code. Also we already have db backup at VP.

@alexjfisher
Copy link
Collaborator

One person's cruft is another's functionality they rely on. Unlike the solaris code, this is out of the way and not interfering with non users.

@LukasAud
Copy link
Contributor

Hi @ghoneycutt, sorry for the long delay in feedback. The ping must have gotten lost in my emails. This is considered a major change and at the moment we do not have the resources to dedicate time to this. It seems to break (understandably) a large amount of test cases in all of our supported OS, which in turn adds to the effort required.

In addition, we are currently not sure if this is the approach we want to follow regarding the backup classes. @alexjfisher makes a good point stating that this might be useful functionality for a portion of the community and/or clients but not all.

The suggested change will require an extensive discussion and/or extra work before we can move ahead and merge/close it.

@fraenki
Copy link
Contributor

fraenki commented Jul 3, 2022

Backups are extremely site specific and having them in their current state is not providing value.

I strongly disagree. They are very useful, at least for our use-cases.

Also we already have db backup at VP.

FWIW, pupppet-dbbackup is not a valid replacement in our case. It requires systemd and does not support xtrabackup.

@LukasAud
Copy link
Contributor

Hi @ghoneycutt. Unfortunately, it doesn't look like we will be able to investigate your PR in the near future, as we are currently focusing our work on other areas. We are also afraid this PR will keep being flagged as inactive by our system.

As such, could you create a feature request in our 'Issues' section instead and then close and link the PR there? That way, this request will remain in our view and any progress/discussion will still be easily accessible.

Sorry for the inconvenience.

@pmcmaw
Copy link
Contributor

pmcmaw commented Oct 5, 2022

Closing in line with above comments.

@pmcmaw pmcmaw closed this Oct 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants