Add tags to identify read-only resources #805

jeskew · 2022-05-13T16:59:22Z

Resolves #411.

This PR adds a Flags property to the serialized bicep resource type to inform the compiler whether resources are read-only, write-only, or support both reading and writing. This will allow the compiler to handle existing resources for read-only types while disallowing their use as regular resource types.

For backwards compatibility, flags only indicate deviations from the norm, so ResourceFlags.None represents a resource that is both readable and writable. The Bicep.Types library, even with this change, will treat existing serialized resources as having a flag of ResourceFlags.None, so there's no need to coordinate a release of that library with one of the k8s types.

src/Bicep.Types/Concrete/ResourceType.cs

src/autorest.bicep/src/resources.ts

src/Bicep.Types/Concrete/ObjectType.cs

codecov-commenter · 2022-05-20T14:49:05Z

Codecov Report

Merging #805 (16b3982) into main (7121acc) will increase coverage by 0.32%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #805      +/-   ##
==========================================
+ Coverage   95.57%   95.89%   +0.32%     
==========================================
  Files          18       18              
  Lines         294      317      +23     
==========================================
+ Hits          281      304      +23     
  Misses         13       13

Flag	Coverage Δ
dotnet	`95.89% <100.00%> (+0.32%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/Bicep.Types.UnitTests/TypeSerializerTests.cs	`100.00% <100.00%> (ø)`
src/Bicep.Types/Concrete/ResourceType.cs	`100.00% <100.00%> (ø)`

jeskew · 2022-05-26T19:57:05Z

I've scoped this PR down to just include ReadOnly and WriteOnly flags on resources. There are a few related concerns that have been spun off into separate issues that warrant some more discussion.

#838 shows the type generation updates that will follow from this PR.

anthony-c-martin · 2022-05-27T02:40:03Z

The changes look sensible - just working through the type generation PR. I thought it would be easiest if I add my comments directly there.

Do you have any ideas on improving the review process for this monstrous set of changes, or suggestions for things to focus on?

anthony-c-martin · 2022-05-27T02:49:08Z

By the way, I would recommend giving the following users/teams a heads-up on these changes:

@tfitzmac - AFAIK he's using this repo for the Template Reference docs generation, and may want to exclude non-PUTable resources.
@grayzu - https://github.com/Azure/terraform-provider-azapi is using this repo to generate TF types.

@alex-frankel are you aware of any other non-obvious dependencies on this repo?

jeskew · 2022-05-27T02:56:55Z

Do you have any ideas on improving the review process for this monstrous set of changes, or suggestions for things to focus on?

Unfortunately, no. The .md files are more compact than the logs, but there are apparently a ton of read-only resources.

tfitzmac · 2022-05-27T15:11:20Z

Thanks for letting me know about this change. Just to make sure I understand it - it looks like many resource types were added that are marked as "(ReadOnly)" at the end of the first line of the resource definition. These seem to all be types that were not appearing before in the generated types. My process will block those types for published docs.

Let me know if any of my assumptions are wrong or if there are other changes I need to be aware of.

jeskew · 2022-05-27T20:22:24Z

By the way, I would recommend giving the following users/teams a heads-up on these changes:

@tfitzmac - AFAIK he's using this repo for the Template Reference docs generation, and may want to exclude non-PUTable resources.

@grayzu - https://github.com/Azure/terraform-provider-azapi is using this repo to generate TF types.

@anthony-c-martin Would it make sense to release new types resulting from this change as version 0.2.0? Azure.Bicep.Types is currently on 0.1.486, and a transition from 0.1.x -> 0.2.x would allow for breaking changes.

…g rebase was too difficult)

…nd a parameterized-name GET (or vice versa)

…to the integration test

…pecific readonly flags

majastrz

… the same definition

anthony-c-martin · 2022-06-24T13:41:58Z

By the way, I would recommend giving the following users/teams a heads-up on these changes:

@tfitzmac - AFAIK he's using this repo for the Template Reference docs generation, and may want to exclude non-PUTable resources.

@grayzu - https://github.com/Azure/terraform-provider-azapi is using this repo to generate TF types.

@anthony-c-martin Would it make sense to release new types resulting from this change as version 0.2.0? Azure.Bicep.Types is currently on 0.1.486, and a transition from 0.1.x -> 0.2.x would allow for breaking changes.

Doesn't hurt!

anthony-c-martin · 2022-06-24T13:50:55Z

src/autorest.bicep/src/type-generator.ts

+      if (ns.type === 'parameterized') {
+        const {schema} = ns;
+        if (schema instanceof ConstantSchema && toBuiltInTypeKind(schema.valueType) === BuiltInTypeKind.String) {
+          nameLiterals.add(schema.value.value);
+        } else if (schema instanceof ChoiceSchema || schema instanceof SealedChoiceSchema) {
+          const enumValues = getValuesForEnum(schema);
+          if (enumValues.success) {
+            const {values, closed} = enumValues.value;
+            values.forEach(v => nameLiterals.add(v));
+            if (!closed) {
+              nameTypes.add(BuiltInTypeKind.String);
+            }
+          }
+        } else {
+          nameTypes.add(toBuiltInTypeKind(schema));
+        }
+      } else {
+        nameLiterals.add(ns.value);
+      }
+    }
+


First off, I apologize for the general lack of comments in this repo. The code is already difficult to follow, and with the handling of more corner cases, it's becoming more complex. No need to address in this PR, but going forwards I think it might be worth thinking about starting to add some comments to explain some of the more complex logic.

I did add one comment for this section, but I agree that it would be a good idea to put more in going forward.

anthony-c-martin

Looks good!

jeskew · 2022-06-24T23:29:57Z

With #874, #884, and #921 in master, the typegen diff (viewable as #838) ended up being a lot more manageable (at least wrt the markdown and log files). I was able to verify that the effect of this PR was almost entirely additive (with 8 existing resources gaining a new ReadOnly scope) and that all of the resources added are ReadOnly.

jeskew requested a review from anthony-c-martin May 13, 2022 16:59

jeskew changed the title ~~Add resource tags~~ Add tags to identify read-only, write-only, and async resources May 16, 2022

anthony-c-martin reviewed May 18, 2022

View reviewed changes

src/Bicep.Types/Concrete/ResourceType.cs Outdated Show resolved Hide resolved

anthony-c-martin reviewed May 18, 2022

View reviewed changes

src/autorest.bicep/src/resources.ts Show resolved Hide resolved

anthony-c-martin reviewed May 18, 2022

View reviewed changes

src/autorest.bicep/src/resources.ts Outdated Show resolved Hide resolved

anthony-c-martin reviewed May 18, 2022

View reviewed changes

src/Bicep.Types/Concrete/ObjectType.cs Outdated Show resolved Hide resolved

jeskew changed the title ~~Add tags to identify read-only, write-only, and async resources~~ Add tags to identify read-only and write-only resources May 26, 2022

jeskew mentioned this pull request May 26, 2022

Generate type definitions for readonly resources #780

Closed

jeskew requested review from a team and anthony-c-martin May 26, 2022 19:57

jeskew force-pushed the jeskew/add-resource-tags branch from 9c7bff1 to 0e3d296 Compare May 27, 2022 22:43

jeskew mentioned this pull request Jun 8, 2022

Sort object types before writing markdown file #874

Merged

jeskew force-pushed the jeskew/add-resource-tags branch from 0e3d296 to c74a5d5 Compare June 8, 2022 21:24

jeskew mentioned this pull request Jun 14, 2022

Run local typegen for comparison #885

Closed

Manually replay resource tagging changes over main (history-preservin…

ace36df

…g rebase was too difficult)

jeskew force-pushed the jeskew/add-resource-tags branch from c74a5d5 to ace36df Compare June 16, 2022 19:51

Put a pin in WriteOnly resources for the time being

9127c68

jeskew changed the title ~~Add tags to identify read-only and write-only resources~~ Add tags to identify read-only resources Jun 17, 2022

jeskew added 5 commits June 20, 2022 12:04

Some ternary tweaks to line up more precisely with existing behavior

46d353c

Deduplicate the few cases where services define a constant-name PUT a…

2c36ebc

…nd a parameterized-name GET (or vice versa)

Add a resource with a constant-name PUT and a parameterized-name GET …

a8905bd

…to the integration test

Add an additional property to serialized resources to capture scope-s…

d43e6b5

…pecific readonly flags

Add a scope-specific readonly flag scenario to integration test

77bd193

Flag this change as necessitating a minor version bump

16b3982

majastrz approved these changes Jun 23, 2022

View reviewed changes

Only perform resource deduplication when the get and put schemata use…

72f4860

… the same definition

anthony-c-martin reviewed Jun 24, 2022

View reviewed changes

anthony-c-martin approved these changes Jun 24, 2022

View reviewed changes

jeskew added 3 commits June 24, 2022 11:22

Remove debugger statement and add a comment

5f63029

Merge branch 'main' into jeskew/add-resource-tags

5d7b502

Merge branch 'main' into jeskew/add-resource-tags

6eccbb1

jeskew merged commit a1344a9 into main Jun 24, 2022

jeskew deleted the jeskew/add-resource-tags branch June 24, 2022 23:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add tags to identify read-only resources #805

Add tags to identify read-only resources #805

jeskew commented May 13, 2022 •

edited

Loading

codecov-commenter commented May 20, 2022 •

edited

Loading

jeskew commented May 26, 2022

anthony-c-martin commented May 27, 2022

anthony-c-martin commented May 27, 2022

jeskew commented May 27, 2022

tfitzmac commented May 27, 2022

jeskew commented May 27, 2022

majastrz left a comment

anthony-c-martin commented Jun 24, 2022

anthony-c-martin Jun 24, 2022

jeskew Jun 24, 2022

anthony-c-martin left a comment

jeskew commented Jun 24, 2022

Add tags to identify read-only resources #805

Add tags to identify read-only resources #805

Conversation

jeskew commented May 13, 2022 • edited Loading

codecov-commenter commented May 20, 2022 • edited Loading

Codecov Report

jeskew commented May 26, 2022

anthony-c-martin commented May 27, 2022

anthony-c-martin commented May 27, 2022

jeskew commented May 27, 2022

tfitzmac commented May 27, 2022

jeskew commented May 27, 2022

majastrz left a comment

Choose a reason for hiding this comment

anthony-c-martin commented Jun 24, 2022

anthony-c-martin Jun 24, 2022

Choose a reason for hiding this comment

jeskew Jun 24, 2022

Choose a reason for hiding this comment

anthony-c-martin left a comment

Choose a reason for hiding this comment

jeskew commented Jun 24, 2022

jeskew commented May 13, 2022 •

edited

Loading

codecov-commenter commented May 20, 2022 •

edited

Loading