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

feat(lib): add move workflows for specifying ids directly #3231

Merged
merged 11 commits into from
Nov 27, 2023
Merged

Conversation

Maed223
Copy link
Contributor

@Maed223 Maed223 commented Nov 2, 2023

Description

Adds a alternative workflow for performing resource moves by specifying resource ids directly.

The original workflow for resource moves didn't have (as of writing this) any bugs per say, yet some of the tradeoffs made in it's design made certain use cases quite difficult. As such, I made resource instance functions where the id to move to/from can be directly specified. Provides a better experience compared to using escape hatches, while avoiding potentially unhelpful abstraction in the targeting system.

In particular we saw in migrating the repository manager to use our new id generation that using the target system was incredibly cumbersome. In response to this use case, I've created an Aspect that will automatically perform the resource moves between the old id generation and the new for ease of upgrading past 0.17.

Resource Instance Function Additions

moveToId(id: string)

Moves the resource it is called upon to the resource corresponding to id

new S3Bucket(this, "test-bucket-moved", {
  bucket: "test-move-bucket-move-to-id",
}).moveToId("aws_s3_bucket.test-bucket-moved-to");

new S3Bucket(this, "test-bucket-moved-to", {
  bucket: "test-move-bucket-move-to-id",
});

moveFromId(id: string)

Move the resource corresponding to id to the resource it is called upon. Note that the resource being moved from must be marked as moved using the instance function .hasMoved().

new S3Bucket(this, "test-bucket-moved-from", {
  bucket: "test-move-bucket-move-from-id",
}).hasMoved();
new S3Bucket(this, "test-bucket", {
  bucket: "test-move-bucket-move-from-id",
}).moveFromId("aws_s3_bucket.test-bucket-moved-from");

Id Migration Aspect

/**
 * For migrating past 0.17 where id generation changed
 */
export class MigrateIdsAspect implements IAspect {
  visit(node: IConstruct) {
    // eslint-disable-next-line no-instanceof/no-instanceof
    if (node instanceof TerraformResource) {
      const oldId = allocateLogicalIdOldVersion(node);
      node.moveFromId(`${node.terraformResourceType}.${oldId}`);
    }
  }
}

Added an Aspect (MigrateIdsAspect) to the lib that migrates ids from prev 0.17 to post 0.17 when the feature flag was removed. Uses the old logic for Id generation to resolve the original resource address and calls moveFromId with that old address.

Should solve our issue in the repository manager quite nicely as well as any users in the same situation of upgrading their CDKTF version.

A screenshot of the new Aspect in action

Screenshot 2023-11-14 at 9 30 23 AM

@Maed223 Maed223 marked this pull request as ready for review November 14, 2023 15:34
@Maed223 Maed223 requested a review from a team as a code owner November 14, 2023 15:34
@Maed223 Maed223 requested review from mutahhir and ansgarm and removed request for a team November 14, 2023 15:34
@Maed223 Maed223 requested a review from a team as a code owner November 16, 2023 22:52
@Maed223
Copy link
Contributor Author

Maed223 commented Nov 16, 2023

Still need to shout out the new MigrateIdsAspect somewhere. Figure the 0.17 upgrade guide would be the logical place, but to me it doesn't seem to be the most visible place. Curious to hear opinions on this 😁

@DanielMSchmidt
Copy link
Contributor

@Maed223 I think the upgrade guide is the best place for that 👍

Copy link
Member

@ansgarm ansgarm left a comment

Choose a reason for hiding this comment

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

Nice work! Looks good to me 🚀
Got a few cleanup items / docs improvements, but would've been an approve otherwise ✅

examples/typescript/aws-move/main.ts Outdated Show resolved Hide resolved
packages/cdktf/lib/terraform-resource.ts Outdated Show resolved Hide resolved
packages/cdktf/lib/terraform-resource.ts Outdated Show resolved Hide resolved
packages/cdktf/lib/terraform-resource.ts Outdated Show resolved Hide resolved
packages/cdktf/lib/terraform-resource.ts Outdated Show resolved Hide resolved
packages/cdktf/lib/upgrade-id-aspect.ts Outdated Show resolved Hide resolved
packages/cdktf/test/resource.test.ts Outdated Show resolved Hide resolved
packages/cdktf/test/resource.test.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@DanielMSchmidt DanielMSchmidt left a comment

Choose a reason for hiding this comment

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

The only "blocking" one is my question in the refactoring examples :)

packages/cdktf/lib/terraform-resource.ts Show resolved Hide resolved
packages/cdktf/lib/terraform-resource.ts Outdated Show resolved Hide resolved
packages/cdktf/test/resource.test.ts Outdated Show resolved Hide resolved
packages/cdktf/test/resource.test.ts Outdated Show resolved Hide resolved
Copy link
Contributor

I'm going to lock this pull request because it has been closed for 30 days. This helps our maintainers find and focus on the active issues. If you've found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 28, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants