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

fix: correctly normalize json types #147

Merged
merged 8 commits into from
Aug 27, 2024
Merged

Conversation

corymhall
Copy link
Contributor

@corymhall corymhall commented Aug 19, 2024

Some properties in aws-native are Json types, meaning that their values
are sent to CloudControl as is. For all other types we do some
conversion between the CloudControl type and the pulumi type.

An example of this is any policy documents, such as the
AWS::S3::AccessPoint.Policy property. The pulumi type is
pulumi.json#/Any. In the below example, you can see the property keys
within policy have their first character uppercase.

new aws.s3.AccessPoint('access-point', {
  policy: {
    Version: '2012-10-17',
    Statement: [
      {
        Action: ['s3:GetObject'],
        ...
      }
    ]
  }
});

In order to handle these types in the normalize function I've added a
utility to parse the aws-native metadata.json schema and lookup the
types.

I've also removed the cfn mappings that exist in order to handle the
json mappings and the existing tests cover it.

closes #127

Some properties in aws-native are Json types, meaning that their values
are sent to CloudControl as is. For all other types we do some
conversion between the CloudControl type and the pulumi type.

An example of this is any policy documents, such as the
`AWS::S3::AccessPoint.Policy` property. The pulumi type is
`pulumi.json#/Any`. In the below example, you can see the property keys
within `policy` have their first character uppercase.

```ts
new aws.s3.AccessPoint('access-point', {
  policy: {
    Version: '2012-10-17',
    Statement: [
      {
        Action: ['s3:GetObject'],
        ...
      }
    ]
  }
});
```

In order to handle these types in the `normalize` function I've added a
utility to parse the aws-native `metadata.json` schema and lookup the
types.

I've also removed the `cfn` mappings that exist in order to handle the
json mappings and the existing tests cover it.

BREAKING CHANGE: `normalize` now takes an additional `cfnType` argument

closes #127
corymhall added a commit that referenced this pull request Aug 20, 2024
This PR updates the workflows and scripts

**Workflows**
- Splits the acceptance tests to run in parallel across 3 workers
- Runs unit tests and acceptance tests in different jobs
- Applies the same changes to the release workflow
- Moves some reusable logic into actions/*/action.yml files

**Others**
- Replaced the `package.json` `files` config with `.npmignore`
- Instead of running npm publish from the `lib` directory, it is now run
  from the root directory (this is in preparation of #147 which needs
  non-lib files included)
- Separated out a `prepare` script from `build` so that build can be run
  locally without updating the `package.json` version field
- Added a `watch` and a `test:watch` script
corymhall added a commit that referenced this pull request Aug 20, 2024
This PR updates the workflows and scripts

**Workflows**
- Splits the acceptance tests to run in parallel across 3 workers
- Runs unit tests and acceptance tests in different jobs
- Applies the same changes to the release workflow
- Moves some reusable logic into actions/*/action.yml files

**Others**
- Replaced the `package.json` `files` config with `.npmignore`
- Instead of running npm publish from the `lib` directory, it is now run
  from the root directory (this is in preparation of #147 which needs
  non-lib files included)
- Separated out a `prepare` script from `build` so that build can be run
  locally without updating the `package.json` version field
- Added a `watch` and a `test:watch` script
- run `yarn link` from the root of the directory instead of `lib`
- This also requires us to pin `aws-cdk-lib` version (devDep in root
package.json, dep in examples) so that the version is the same between
`@pulumi/cdk` and the examples. Different versions cause type errors.
@corymhall corymhall self-assigned this Aug 21, 2024
@corymhall corymhall requested review from t0yv0 and flostadler August 23, 2024 17:30
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reminder to myself to create a backlog issue for creating a workflow to update this on a schedule.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why don't we trigger an update whenever we're cutting a release to aws-native?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah we could do that too.

@@ -95,26 +346,3 @@ export class CdkConstruct extends pulumi.ComponentResource {
this.registerOutputs({});
}
}

export class CdkComponent extends pulumi.ComponentResource {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was unused, so removing it.

Copy link
Contributor

Choose a reason for hiding this comment

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

nice!

@corymhall corymhall changed the title fix!: correctly normalize json types fix: correctly normalize json types Aug 23, 2024
Copy link
Contributor

@flostadler flostadler left a comment

Choose a reason for hiding this comment

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

Left some questions.

I have two more generic items I was wondering about:

  • Don't we have to ensure the aws-native version and the version we're using for the metadata json match? We generally pass breaking changes in the cfn schema through to the users of aws-native. Wouldn't this lead to issues here? Kind of feels like we'll need to tackle the type freezing in aws-native (Implement type freezing mechanism pulumi-aws-native#1684)
  • I'm wondering if we should use git submodules for the aws-native-metadata.json file. Right now we don't know what version of aws-native it actually refers to. This would also make future upgrade reviews easier.

src/interop.ts Outdated Show resolved Hide resolved
src/interop.ts Outdated Show resolved Hide resolved
src/interop.ts Outdated
import * as path from 'path';
import * as fs from 'fs';

const glob = global as any;
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the reason for having global metadata? I assume it's to not load the aws-native-metadata.json multiple times?

What would happen if two pulumi cdk programs are used in automation API and they have different pulumi cdk versions? Wouldn't that mean that there's a chance that the newer program falls back to the metadata of the old one; possibly causing failures?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah maybe i'll just try and read it one time per execution instead of doing it this way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@flostadler I've updated this to instead just require() the file.

src/interop.ts Outdated Show resolved Hide resolved
src/interop.ts Outdated
return { nativeType: NativeType.JSON };
case property.$ref?.startsWith('pulumi.'):
return { nativeType: NativeType.NON_JSON };
case lastProp:
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is the last prop automatically non json? Or is that just to make sure the prop will be processed later?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this was left over from an earlier iteration. I've removed the logic around lastProp

src/interop.ts Outdated Show resolved Hide resolved
src/interop.ts Outdated
} catch (e) {
debug(`error reading pulumi schema: ${e}`);
// fallback to processing without the schema
return normalizeGenericResourceObject(key, value);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it fine if we continue here? Could anything possibly go wrong (e.g. unexpected deletes)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Things might go wrong, but it will be no worse than the current implementation.

The only time it should realistically hit this is when you are normalizing properties for a non-aws-native resource (e.g. aws classic). In those cases the best we can do is just normalize everything. We could also add similar logic for processing the aws classic schema and then turn this into a hard failure.

@@ -95,26 +346,3 @@ export class CdkConstruct extends pulumi.ComponentResource {
this.registerOutputs({});
}
}

export class CdkComponent extends pulumi.ComponentResource {
Copy link
Contributor

Choose a reason for hiding this comment

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

nice!

src/interop.ts Outdated Show resolved Hide resolved
src/interop.ts Outdated
items?: PulumiPropertyItems;
}

interface PulumiResource {
Copy link
Member

Choose a reason for hiding this comment

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

Is this info coming from a Pulumi Package Schema, implicitly? E.g. that's the format with "$ref" references and such?

I spent some time thinking about how this fits here, we're in a rather unfortunate situation that all nicer-to-use models for handling Pulumi Package Schema information are written in Go. This code looks like it's starting to build one in TS. Perhaps we just have to do this?

What could be an alternative? I guess we could do some more static analysis in Go at build-time and pre-compute some easy-to-use runtime metadata for TS. Does't necessarily sound very attractive to me, but listing as an option.

If we go with formalizing Pulumi Package Schema in TS I think we could lean harder in TS typesystem to make code airtight type-safe, but we can start easy and iterate on this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this info coming from a Pulumi Package Schema, implicitly? E.g. that's the format with "$ref" references and such?

Yes.

If we go with formalizing Pulumi Package Schema in TS I think we could lean harder in TS typesystem to make code airtight type-safe, but we can start easy and iterate on this.

I think it would be nice to have this in all languages that pulumi supports building component libraries in. For my purposes I only need some of the information from the schema so I think it should be out of scope to build the complete typescript schema.

src/interop.ts Outdated
}

Object.entries(value).forEach(([k, v]) => {
result[toSdkName(k)] = normalizeObject([...key, k], v, cfnType);
Copy link
Member

Choose a reason for hiding this comment

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

I'm thinking toSdkName is probably approximate, but still not 100% sure. Some corner cases likely lurking. I've been reading this code recently:

https://github.com/pulumi/pulumi-aws-native/blob/master/provider/pkg/naming/naming.go#L24

And ToCfnName seems to need a table. But ToSdkName doesn't? That's very interesting. I suspect as the system grows both will need to consult tables from the native metadata. We can leave for later though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was looking into the aws-native implementation as well. I'll add a backlog item for making sure the the pulumi-cdk renaming libraries match aws-native.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually I think I can just handle this in this PR. I forgot that I already did the conversion in an earlier PR, but without the lookupTable because we didn't have the metadata which we now do!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nvm I got confused. toSdkName doesn't require a lookup table and it is the only one currently in use in this library.

- Updated/pinned `@pulumi/aws-native` to `0.121.0` and synced
  `aws-native-metadata.json`
- `require()` the metadata file instead of using a global variable
- added more tests
- reorganized the new code
@corymhall corymhall requested a review from flostadler August 26, 2024 18:04
@corymhall corymhall merged commit 1094919 into main Aug 27, 2024
8 checks passed
@corymhall corymhall deleted the corymhall/normalize-json branch August 27, 2024 12:37
@pulumi-bot
Copy link
Contributor

This PR has been shipped in release v1.0.0.

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.

Automatically map properties with Json types
4 participants