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

Add backup_config resource #241

Closed
wants to merge 1 commit into from

Conversation

fantapop
Copy link
Collaborator

@fantapop fantapop commented Oct 18, 2024

Note that this commit is still a work in progress as the final SDK changes required for its use are not deployed yet but I've tested it locally and it's in a good place for feedback.

@mdlinville I was particularly hoping to get your feedback on the documentation that I added. This ended up being a documentation heavy resource.

Commit checklist

  • Changelog
  • Doc gen (make generate)
  • Integration test(s)
  • Acceptance test(s)
  • Example(s)

I'm not super clear on how well the large block of documentation will render on the terraform docs site. Here is the how the value looks with my local rendering. Note: I've updated this screenshot after the latest batch of edits.

Screenshot 2024-10-22 at 1 18 53 PM

@fantapop fantapop force-pushed the fantapop/CC-27591-backup-configuration branch from a5d3496 to 48c107e Compare October 18, 2024 20:11
@mdlinville mdlinville requested a review from kathancox October 21, 2024 20:23
Copy link

@kathancox kathancox left a comment

Choose a reason for hiding this comment

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

Added some comments for the doc component of this. I wasn't sure of the constraints here, so feel free to take the comments as you need. I can take another look when you need it.

docs/resources/backup_config.md Outdated Show resolved Hide resolved
docs/resources/backup_config.md Outdated Show resolved Hide resolved
docs/resources/backup_config.md Outdated Show resolved Hide resolved
docs/resources/backup_config.md Outdated Show resolved Hide resolved
docs/resources/backup_config.md Outdated Show resolved Hide resolved
docs/resources/backup_config.md Outdated Show resolved Hide resolved
docs/resources/backup_config.md Outdated Show resolved Hide resolved
docs/resources/backup_config.md Outdated Show resolved Hide resolved
docs/resources/backup_config.md Outdated Show resolved Hide resolved
docs/resources/backup_config.md Show resolved Hide resolved
@fantapop fantapop marked this pull request as ready for review October 22, 2024 01:43
@fantapop fantapop force-pushed the fantapop/CC-27591-backup-configuration branch from 48c107e to 5a50b99 Compare October 22, 2024 01:44
@fantapop
Copy link
Collaborator Author

@kathancox I made a round of changes. You can see the diff here: https://github.com/cockroachdb/terraform-provider-cockroach/compare/48c107eae8a9bdc446d998fda84c1b9d1daa4d93..5a50b9916d3b01d54a25b5ee51a051f2d4bc34c9

I think I've incorporated everything but I may have missed something. Please feel free to make more comments if you have further edits.

Copy link

@kathancox kathancox left a comment

Choose a reason for hiding this comment

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

The back_config.md doc lgtm

We add the cockroach_backup_config resource to manage backup
configuration for clusters.
@fantapop fantapop force-pushed the fantapop/CC-27591-backup-configuration branch from 5a50b99 to 799233f Compare October 22, 2024 20:17
@fantapop fantapop changed the title WIP: Add backup_config resource Add backup_config resource Oct 22, 2024
@fantapop
Copy link
Collaborator Author

I was just able to do some local testing of this and it's performing as expected. One oddity which I called out in the behavior document I created is that when you remove the resource, no delete actually occurs. It's just removing management of the backup configuration. This may be slightly surprising to a user but It's probably an unlikely scenario to run into. A terraform removal looks like this in the CLI.

Screenshot 2024-10-22 at 5 03 08 PM

I think this is okay and is probably our best option here. A couple other approaches I could think of:

  1. have backups disabled by default and have the lack of a resource indicate that they are disabled. This would mean that addition and removal of the config aligns with whether they are on or off. That seems undesirable because disabled by default is hostile to the user.
  2. have backup configuration be a setting on the cluster resource. In this case, we're not adding or removing a resource in terraform which correlates better to backup configuration always existing. This is a potentially viable option and may be more natural for a user. The 2 main drawbacks here are 1. The cluster resource is quite complex already 2. I didn't consider this before the existing approach so migrating to the other approach would be more effort.

@fantapop
Copy link
Collaborator Author

  1. have backup configuration be a setting on the cluster resource. In this case, we're not adding or removing a resource in terraform which correlates better to backup configuration always existing. This is a potentially viable option and may be more natural for a user. The 2 main drawbacks here are 1. The cluster resource is quite complex already 2. I didn't consider this before the existing approach so migrating to the other approach would be more effort.

I've decided to dedicate a couple hours to exploring this.

@fantapop
Copy link
Collaborator Author

closing in favor of #242

@fantapop fantapop closed this Oct 28, 2024
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

Successfully merging this pull request may close these issues.

3 participants