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

Improvements to C# Ergonomics #3019

Open
1 task done
BrentSouza opened this issue Jul 21, 2023 · 1 comment
Open
1 task done

Improvements to C# Ergonomics #3019

BrentSouza opened this issue Jul 21, 2023 · 1 comment
Labels
enhancement New feature or request help wanted Community contributions welcome as the core team is unlikely to work on this soon language/c# C# language issues needs-research priority/awaiting-more-evidence Lowest priority. Unlikely to be worked on unless/until it gets a lot more upvotes. ux/configuration

Comments

@BrentSouza
Copy link

Description

I raise this issue with constructive intent, and I realize it may not align with the priority or direction for the overall project, but since C# is a supported language, and this is the internet afterall, I wanted to share my thoughts.

I think it's prudent to note that the C# developer experience is not great, but there are some quality of life changes that could be made to improve it. I am not sure if these improvements need to be made in cdktf or some of the dependent libraries like
constructs or jsii. I'll happily open issues in all locations where appropriate and contribute where supported.

Overall, the topics discussed below come from the following mindset:

If cdktf is a development oriented approach to managing terraform resources, it makes sense to align with the typical developer experience in the target language.

To contextualize my opinions below, I have been using terraform since 0.11 and devloping in C# for 15 years. My organization controls all of our infrastructure with terraform and we've done a few experiments with cdktf in TypeScript, Python, and C#. I
am not saying this to brag. I don't think it's that special. I just want to share that I do have relevant experience using the tooling.

Linting

# pragma warning disable CA1806 is required in order to avoid warnings about new'ing up objects without assignment. I know this has been reported and is part of the underlying pattern for constructs. However, I think this warning exists for a good
reason and ignoring it feels off for the developer experience.

A quick fix would be to provide a default suppression of the warning with the project template. I don't think this is the only solution, and I'll touch upon that in other sections.

Incomplete Typing

A lot of the types are incomplete or incorrect, which looks to be a side effect of IResolvable as from what I have seen, these properties in TypeScript are all unions of <someType> | cdktf.IResolvable

var zoneConfig = new DataAwsRoute53ZoneConfig
{
    Name = "foo.com.",
    PrivateZone = false // PrivateZone is typed as an object
}

var policyConfig = new DataAwsIamPolicyDocumentConfig
{
    Version = "2012-10-17",
    Statement = new DataAwsIamPolicyDocumentStatement // Statement is typed as object
    {
        Sid = "SomeThingDescriptive",
        Actions = new[]{ ... },
        Effect = "Allow",
        Resources = new[]{ ... }
    }
};

There are no unions in C# so maybe object is the only choice if this capability is needed. If it's possible to make IResolvable a generic interface, that might allow you to translate into C# with something like:

public class Intrinsic<T> : IResolvable<T>
{
    private readonly T _value;
    public Intrinsic(T value)
    {
        _value = value;
    }
    public T Resolve(IResolveContext context) => _value;
}

public static class Resolvable
{
    public static IResolvable<T> FromValue<T>(T value) => new Intrinsic<T>(value);
}

public class SomeConfig
{
    public IResolvable<bool> SomeProp { get; init; }
    public IResolvable<DataAwsIamPolicyDocumentStatement> AnotherProp { get; init; }
}

public class Program
{
    public static int Main()
    {
        SomeConfig config = new()
        {
            SomeProp = Resolvable.FromValue(true),
            AnotherProp = Resolvable.FromValue(new()
            {
                ...
            })
        };
        ...
    }
}

The above keeps the desired type visible and gives the developer a reasonable chance at knowing allowed values without digging into any external documentation.

It could be less verbose if there's a concrete expression type that could be used for properties. That would allow for an implict cast:

public class AttributeExpression<T> : IResolvable<T>
{
    private readonly Lazy<T> _value;

    public AttributeExpression(Lazy<T> value)
    {
        _value = value;
    }

    public T Resolve(IResolveContext context) => _value.Value;

    public static implicit operator AttributeExpression<T>(T value) => new(new Lazy<T>(value));
}

public class SomeConfig
{
    public AttributeExpression<bool> SomeProp { get; init; }
    public AttributeExpression<DataAwsIamPolicyDocumentStatement> AnotherProp { get; init; }
}

public class Program
{
    public static int Main()
    {
        SomeConfig config = new()
        {
            SomeProp = true,
            AnotherProp = new DataAwsIamPolicyDocumentStatement()
            {
                ...
            }
        };
        ...
    }
}

Namespaces

The namespacing is pretty verbose. I read the pinned issue about how this was changed in cdktf 0.13 and understand the reasoning around performance.

In addition to pulling the schema from the provider, would it make sense to use the registry api to pull the doc navigation and use the subcategory field to drive the namespacing? It seems like there is some existing metadata that could be used to scope the nodes within a namespace without requiring lots of effort.

Surprising Implementations

For example List types don't implement list semantics. If we look at AcmCertificate, it has a property DomainValidationOptions which is of the type AcmCertificateDomainValidationOptionsList. This is indeed a list in terraform, where a common pattern of consumption is to use a for expression to create corresponding route53 records to validate the domains on the cert, as in the example from the tf docs.

This is not a list in C#. I can't get a count, I can't use any of the System.Linq extension methods, and I can't evaluate it in a loop. I can only index into it via the Get method:

var dvo0 = cert.DomainValidationOptions.Get(0);
var dvo1 = cert.DomainValidationOptions.Get(1);

Ideally this would implement IList<AcmCertificateDomainValidationOptionsOutputReference>, and we could use it like a typical list in C#.

Interfaces for Configs

I understand that the interfaces make sense in TypeScript since they are evaluated against an anonymous object. This is not the case in C#, which makes the resource creation more explicit. This could be better if the resource constructors used the
concrete type, e.g.:

public class FooResource
{
    public FooResource(Construct scope, string id, FooResourceConfig config)
    {
        ...
    }
}
// versus
public class BarResource
{
    public BarResource(Construct scope, string id, IBarResourceConfig config)
    {
        ...
    }
}

Newer versions of C# allow you to omit the class when the type is inferred. So that would allow us to do:

FooResource foo = new(scope, id, new(){ ... });
// versus
BarResource bar = new(scope, id, new BarResourceConfig(){ ... });

Admitidly, the above syntax is controversial in terms of readability, but it's nice to have the option to take advantage of the language features.

Patterns

Coming back to the pattern of new'ing up resources...

It seems more natural to think of the stack as an object to which you are adding resources. Likewise, I would expect to add stacks to an App. I think the inversion of control in place right now drives the awkward experience. I realize this is just
following the Constructs library but I it could be masked with a little syntax:

// Interface to mark any object that has a configuration object as part of its constuctor
public interface IConfigurable<TConfig> {}

// Interface to mark the configuration object and relate it to the object it configures
public interface IConfigure<TElement> {}

// Make TerraformElement generic on the config type
public class TerraformElement<TConfig> : Construct, IConfigurable<TConfig> {}

// Concrete objects can just add the interface directly
public class TerraformAsset : Construct, IConfigurable<TerraformAssetConfig> {}

// Objects meant to be inherited should just be generic on the config as well 
public class TerraformProvider<TConfig> : TerraformElement<TConfig> {}

// Add a new method to TerraformStack to create and add related Elements and
// Configuration objects. The type constraints will help enforce safety of this method
// so you can't use a FooConfig with a BarProvider
public class TerraformStack
{  
    public TElement Add<TElement, TConfig>(string id, TConfig config)
        where TElement : IConfigurable<TConfig>
        where TConfig : IConfigure<TElement>
    {
        var element = (TElement)Activator.CreateInstance(typeof(TElement), this, id, config);
        return element;
    }
}

// Likewise add a new method to App to create and add a stack
public class App
{
    public App AddStack<T>(string id) where T : TerraformStack
    {
        var _ = Activator.CreateInstance(typeof(T), this, id);
        return this;
    }
}

// Imagine this is a configuration object for my custom Some provider
// It is linked to the provider via the interface
public class SomeConfig : IConfigure<SomeProvider> {}

// And this is the provider object itself, also linked back to it's config type
public class SomeProvider : TerraformProvider<SomeConfig> {}

// Now a stack implementation could look something like this
// You can still new things up directly if you wanted
public class SomeStack : TerraformStack
{
    public SomeStack(Construct scope, string id) : base(scope, id)
    {
        var someProvider = Add<SomeProvider, SomeConfig>("some", new(){});
        var someResource = Add<SomeResource, SomeResourceConfig>("some", new()
        {
            Provider = someProvider,
            ...,
        });

        Add<SomeDependentResource, SomeDependentResourceConfig>("some", new()
        {
            Provider = someProvider,
            SomeResourceId = someResource.Id,
            ...
        });
    }
}

public class Program
{
    var app = new App();
    app.AddStack<SomeStack>("foo").Synth();
}

If you read this far, I appreciate it and I hope you consider some of the feedback above. As I mentioned at the beginning of the post, I'm happy to contribute if any of this is possible/acceptable to do within the scope of the project.

References

No response

Help Wanted

  • I'm interested in contributing a fix myself

Community Note

  • Please vote on this issue by adding a 👍 reaction to the original issue to help the community and maintainers prioritize this request
  • Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for issue followers and do not help prioritize the request
  • If you are interested in working on this issue or have submitted a pull request, please leave a comment
@BrentSouza BrentSouza added enhancement New feature or request new Un-triaged issue labels Jul 21, 2023
@ansgarm
Copy link
Member

ansgarm commented Jul 27, 2023

Hi @BrentSouza 👋

First: Wow, thank you for this extensive feedback. We truly appreciate it and are thankful for the time you spent on this 👏

Linting

We'd be open to a PR for a quick fix disabling that lint rule. And yes, this had been brought up before (for cross reference: my reply back then).

Incomplete Typing

This is an interesting proposal and I did a bit of digging and it seems that it aligns with an RFC on JSII that intends to fix the type union problem 😄

Small excerpt from their proposal: LoggingConfigurationPropertyValue.fromIResolvable(lazyValue)

One current limitation to note: JSII doesn't support generics today.

Namespaces

We agree about the verbosity with having one namespace per resource. When we worked on the performance improvement, we also considered using the grouping from the Terraform Registry docs sidebar for provider namespaces. The reason we decided against that, was that changes to that grouping are not a breaking change for Terraform Providers since they are only reflected in docs but not in the Terraform config written while they would be a breaking change for CDKTF provider bindings, unfortunately.

Surprising Implementations

This is due to the fact that the length and contents of these list properties will only be known at "apply-time" which is when Terraform runs and after synth finished. This means that we can't know the length of a list at "synth-time" and the .Get(0) access will actually return a Reference token which will eventually render domain_validation_options[0]. While we could implement length on that same list interface and make it a number token that eventually renders length(xy.domain_validation_options) this could lead to some false sense of security since number tokens are a very large negative number that not actually represents the length but rather a reference in a lookup map.

For list values that can be known at synth time (think inputs to resources), it is possible to access them using the .someAttributeInput property and use native loops with them.

For dynamic values, we added the Iterators feature, but I agree that a simple for-loop would be nicer to use.

Interfaces for Configs

I think this would be something to raise in JSII – but everything that makes code easier to write (even if it might be a bit controversial) is something that has my support.

Patterns

While I don't know much on how the currently used pattern for defining stacks and resources came to be, I suspect that one aspect was also to not have vastly different code across languages (at the expense of being slightly less idiomatic). And with generics being not available in JSII, I suspect that the new pattern might be one common denominator that works across all languages and doesn't require generics.

Thanks again for all that valuable feedback!

@ansgarm ansgarm added needs-research language/c# C# language issues ux/configuration and removed new Un-triaged issue labels Jul 27, 2023
@xiehan xiehan added help wanted Community contributions welcome as the core team is unlikely to work on this soon priority/awaiting-more-evidence Lowest priority. Unlikely to be worked on unless/until it gets a lot more upvotes. labels Oct 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Community contributions welcome as the core team is unlikely to work on this soon language/c# C# language issues needs-research priority/awaiting-more-evidence Lowest priority. Unlikely to be worked on unless/until it gets a lot more upvotes. ux/configuration
Projects
None yet
Development

No branches or pull requests

3 participants