Skip to content

Conversation

mauleb
Copy link
Contributor

@mauleb mauleb commented Jan 1, 2024

Howdy

Some context for this reasonably large refactor:

  • There appears to be mostly consistent issues with the CRD transpiler with 8.0.0-pre.40 as communicated [bug]: System.Int32 is not a valid Kubernetes entity (inconsistently) #687
  • This issue was not limited to int/int? alone after further investigation, but instead a whole slew of types (DateTime,bool,bool?,etc)
  • long story short, type equality checks were the culprit (and I have no idea why). My best guess is that something is off with my machine.
  • While switching out conditions, I noticed opportunities to simplify

I packaged up the code and published it to a local nuget server before installing it in a project. I was able to build the CRD for this entity without any issues with these changes (functionally the kubebuilder book CronJob example):

[k8s.Models.KubernetesEntity(Group = Constants.Operator.Group, ApiVersion = "v1", Kind = "CronJob")]
public partial class V1CronJob : CustomKubernetesEntity<V1CronJob.EntitySpec, V1CronJob.EntityStatus> {
    public class EntitySpec {
        [Required]
        public string Schedule { get; set; } = string.Empty;
        [JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)]
        public DateTimeOffset? StartingDeadlineSeconds { get; set; }
        [AllowedValues("Allow","Forbid","Replace")]
        public string? ConcurrencyPolicy { get; set; }
        [JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)]
        public bool? Suspend { get; set; }
        [Required]
        [EmbeddedResource]
        public k8s.Models.V1JobTemplateSpec JobTemplate { get; set; } = new();
        [JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)]
        public int? SuccessfulJobsHistoryLimit { get; set; } = 3;
        [JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)]
        public int? FailedJobsHistoryLimit { get; set; } = 1;
    }

    public class EntityStatus {
        [JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)]
        public string? LastScheduleTime { get; set; } = default;
        [JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)]
        public List<k8s.Models.V1ObjectReference> Active { get; set; } = new();
    }

    public static class ConcurrencyPolicy {
        public const string Allow = "Allow";
        public const string Forbid = "Forbid";
        public const string Replace = "Replace";
    }
}

this produced the following yaml:

openAPIV3Schema:
  properties:
    status:
      nullable: false
      properties:
        lastScheduleTime:
          nullable: true
          type: string
        active:
          items:
            properties:
              apiVersion:
                nullable: false
                type: string
              fieldPath:
                nullable: false
                type: string
              kind:
                nullable: false
                type: string
              name:
                nullable: false
                type: string
              namespace:
                nullable: false
                type: string
              resourceVersion:
                nullable: false
                type: string
              uid:
                nullable: false
                type: string
            type: object
          nullable: false
          type: array
      type: object
    spec:
      nullable: false
      properties:
        schedule:
          nullable: false
          type: string
        startingDeadlineSeconds:
          format: date-time
          nullable: true
          type: string
        concurrencyPolicy:
          nullable: true
          type: string
        suspend:
          nullable: true
          type: boolean
        jobTemplate:
          nullable: false
          type: object
          x-kubernetes-embedded-resource: true
          x-kubernetes-preserve-unknown-fields: true
        successfulJobsHistoryLimit:
          format: int32
          nullable: true
          type: integer
        failedJobsHistoryLimit:
          format: int32
          nullable: true
          type: integer
      required:
      - schedule
      - jobTemplate
      type: object
  type: object

I also added decimal & DateTimeOffset support due to presence here:

DISCLAIMER: I have no idea what the CRD yaml output should look like. My main goal was to honor the current test suite.

Let me know if there is anything I can do for this PR, but the CRD gen is largely not usable at the moment for me, and I'm still very much confused why that is the case and why there appears to be no mention of the same issue from others.

Thanks

@buehler
Copy link
Collaborator

buehler commented Jan 3, 2024

Hey @mauleb

Thank you for your contribution!
It seems pretty OK if the test solution says so ;-)
I'll need some time to go over the changes. It is quite weird that sometimes it works and sometimes not. The whole magic with loading assemblies is new to me, so there definitely are some bugs.

@buehler
Copy link
Collaborator

buehler commented Jan 16, 2024

You sir, are a genius.

I had the same issues, but very inconsistently as well. As soon as I made a "dotnet clean" and "dotnet build" it worked. I had little to no chance to test it.

However there were issues with the typecheck. I tried to use fixed types but since they are loaded via metadata load context, it is not "the same" type.

Your solution is elegant and solves the issue perfectly. Since the tests are "all green", there should not be an issue since the YAML generated is the same.

Thanks for the contribution!

@buehler buehler merged commit d81a379 into dotnet:main Jan 16, 2024
buehler pushed a commit that referenced this pull request Jan 17, 2024
Fix inconsistently thrown errors that system int (or other types)
are not correct kubernetes types.
buehler pushed a commit that referenced this pull request Jan 17, 2024
Fix inconsistently thrown errors that system int (or other types)
are not correct kubernetes types.
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.

2 participants