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

Implement deletion protection #367

Merged
merged 14 commits into from
Jul 18, 2024

Conversation

jhamon
Copy link
Collaborator

@jhamon jhamon commented Jul 18, 2024

Problem

Implement deletion protection for python sdk

Solution

  • For the basic functionality, needed to update the create index and configure index actions
  • The UX around the generated enum class, DeletionProtection, seemed extremely poor so I added a custom wrapper class IndexModel and incorporated it into the describe_index and list_indexes actions
  • Update and add new tests
  • Remove export of the generated IndexModel from the top-level __init__.py file.

This ended up being a much bigger lift than expected, but as a nice side effect we're no longer exposing generated objects in responses from these various actions. That creates some additional flexibility for future refactoring work.

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update
  • Infrastructure change (CI configs, etc)
  • Non-code change (docs, etc)

Test Plan

Describe specific steps for validating this change.

@jhamon jhamon changed the base branch from main to release-candidate/2024-07 July 18, 2024 14:47
Comment on lines +299 to +300
spec=ServerlessSpec(cloud="aws", region="us-west-2"),
deletion_protection="enabled"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is docstrings code.

Comment on lines +330 to +333
if deletion_protection in ["enabled", "disabled"]:
dp = DeletionProtection(deletion_protection)
else:
raise ValueError("deletion_protection must be either 'enabled' or 'disabled'")
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The generated DeletionProtection class implements a validation, but the error message it emits is very confusing. So I implemented my own check on this here and in the configure method.

Comment on lines +579 to +581
if deletion_protection is None:
description = self.describe_index(name=name)
dp = DeletionProtection(description.deletion_protection)
Copy link
Collaborator Author

@jhamon jhamon Jul 18, 2024

Choose a reason for hiding this comment

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

If the user does not specify a value for deletion_protection, we call describe_index to look up the current value and pass that value with the other arguments in the configure request. Even though the API doesn't require a value to be passed, manual testing showed the generated code will stupidly plugin the default value of disabled if none is specified. This is needed to avoid disabling deletion protection when somebody is trying to scale by calling this with replicas or pod_type only.

pod_config_args.update(replicas=replicas)

if pod_config_args != {}:
spec = ConfigureIndexRequestSpec(pod=ConfigureIndexRequestSpecPod(**pod_config_args))
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I hate the arrangement of this code. Blame the black formatter. I need to update the configs to shorten the line length to something more reasonable.

@jhamon jhamon requested a review from austin-denoble July 18, 2024 21:53
@jhamon jhamon marked this pull request as ready for review July 18, 2024 21:53
Copy link
Contributor

@austin-denoble austin-denoble left a comment

Choose a reason for hiding this comment

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

LGTM, thanks again for flagging the issues around the deletionProtection enum early. Made it much easier to test things across the other clients. 🎉 🚢

It might warrant a broader discussion at some point as to whether we want to completely replace generated types for things like responses in order to give us more flexibility and control over the interface, but that's probably a pretty big lift.

@jhamon jhamon merged commit 5854471 into release-candidate/2024-07 Jul 18, 2024
83 checks passed
@jhamon jhamon deleted the jhamon/deletion-protection branch July 18, 2024 23:41
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.

2 participants