Skip to content
This repository has been archived by the owner on Dec 15, 2022. It is now read-only.

Support configuring the json tags of a field #307

Open
Feggah opened this issue Oct 23, 2022 · 0 comments
Open

Support configuring the json tags of a field #307

Feggah opened this issue Oct 23, 2022 · 0 comments
Labels
enhancement New feature or request

Comments

@Feggah
Copy link
Member

Feggah commented Oct 23, 2022

What problem are you facing?

Currently, when an MR is generated from Terrajet, the JSON tags are a consequence of the field type. If it is required, it doesn't have omitempty. If it's optional, it has the omitempty tag. Here is the code that decides it:

if f.Schema.Optional {
r.paramTags = append(r.paramTags, fmt.Sprintf(`json:"%s" tf:"%s"`, f.JSONTag, f.TFTag))
} else {
// Required fields should not have omitempty tag in json tag.
// TODO(muvaf): This overrides user intent if they provided custom
// JSON tag.
r.paramTags = append(r.paramTags, fmt.Sprintf(`json:"%s" tf:"%s"`, strings.TrimSuffix(f.JSONTag, ",omitempty"), f.TFTag))

This is fine for most use cases for managed resources. But there are some APIs and fields that would be benefited if this behavior can be configured.


The practical use case that I have for this is with the BranchProtection managed resource from GitHub terraform provider.

apiVersion: branchprotection.github.jet.crossplane.io/v1alpha1
kind: BranchProtection
metadata:
  name: main-branch
spec:
  deletionPolicy: Delete
  forProvider:
    repositoryId: my-repository
    pattern: main
    requiredStatusChecks:
      - strict: false
        contexts:
          - my-pipeline-check
  providerConfigRef:
    name: github-jet-provider-config

In the YAML above, the spec.forProvider.requiredStatusChecks[0].contexts field is optional (so the JSON tag has the omitempty value). It configures which status checks are mandatory to pass before a PR merge can be done.

image

By applying the YAML above, we expect that the status check my-pipeline-check MUST pass before any PR could be merged. This works as expected.

The problem is that, if I change the contexts field to contexts: [] -- for example:

apiVersion: branchprotection.github.jet.crossplane.io/v1alpha1
kind: BranchProtection
metadata:
  name: main-branch
spec:
  deletionPolicy: Delete
  forProvider:
    repositoryId: my-repository
    pattern: main
    requiredStatusChecks:
      - strict: false
        contexts: []
  providerConfigRef:
    name: github-jet-provider-config

I would expect that the external API would also have an empty list of status checks. But the Managed Resource code doesn't remove the my-pipeline-check status check from the API.

How could Terrajet help solve your problem?

My hypothesis is that the code is considering the contexts field as not_specified, so it isn't actively forcing the BranchProtection (in the external API) to not have the contexts field filled.

If the hypothesis is correct, we could configure this tags and the code could differ from not_specified, specified and empty_value and configures the external API accordingly.

@Feggah Feggah added the enhancement New feature or request label Oct 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

1 participant