adapter: handle easy objects using catalog implications #34358
Conversation
This one is really a no-op, for now, because secrets are stored in the controller even _before_ the catalog transaction.
aljoscha
changed the title
aljoscha
force-pushed
the
adapter-implications-follow-up
branch
from
3833722 to
f8590a8
Compare
| ); | ||
| // Connection alterations (like rotate keys) are handled via | ||
| // secrets_controller without catalog changes to the connection | ||
| // details structure, so no action needed here. |
There was a problem hiding this comment.
I'm not familiar with these things, but I see a ConnectionDetails::AwsPrivatelink code block in sequence_alter_connection_stage_finish, which does stuff with the cloud_resource_controller. (It seems similar to the migrated code block from sequence_create_connection_stage_finish.) Should that be also migrated here, similar to vpc_endpoints_to_create?
There was a problem hiding this comment.
Indeed, the main difference seems to be that the code "transaction side effects" are not included in a catalog_transact_with_side_effects call, but are run after the transaction. Which... I think is functionally the same?
| // No action needed: altering a secret updates the payload via | ||
| // secrets_controller.ensure() without a catalog transaction, | ||
| // so we shouldn't see AlterSecret updates here. |
There was a problem hiding this comment.
Seems worth adding a soft assert or error log, so we can catch if this ever changes?
| tracing::warn!(?err, "failed to ensure vpc endpoint!"); | ||
| } | ||
| } | ||
| } else { | ||
| tracing::warn!( |
There was a problem hiding this comment.
I know both of these are warns in the original code as well, but I'm wondering if we shouldn't make them errors instead? Seems like we should complain pretty loudly if we fail here, since we've already reported to the user that the command was successful.
| ); | ||
| // Connection alterations (like rotate keys) are handled via | ||
| // secrets_controller without catalog changes to the connection | ||
| // details structure, so no action needed here. |
There was a problem hiding this comment.
Indeed, the main difference seems to be that the code "transaction side effects" are not included in a catalog_transact_with_side_effects call, but are run after the transaction. Which... I think is functionally the same?
3 participants
Motivation
Tips for reviewer
Checklist
$T ⇔ Proto$Tmapping (possibly in a backwards-incompatible way), then it is tagged with aT-protolabel.