Skip to content
This repository has been archived by the owner on Jun 28, 2024. It is now read-only.

WIP try flexible enums #134

Closed
wants to merge 1 commit into from
Closed

WIP try flexible enums #134

wants to merge 1 commit into from

Conversation

ctreatma
Copy link
Contributor

This is an example of how we could address concerns laid out in #124 without introducing the validation failures mentioned in equinix-labs/metal-java#113.

Some things to do:

  • Put together an example of how client code works with the updated structure to see how painful it is
  • See how this spec change impacts metal-java to decide if it's worth upstreaming these changes & adopting this pattern for enums (as opposed to defining enum properties as plain strings instead)

@github-actions
Copy link
Contributor

Hey there and thank you for opening this pull request! 👋🏼

We require pull request titles to follow the Conventional Commits specification and it looks like your proposed title needs to be adjusted.

Details:

No release type found in pull request title "WIP try flexible enums". Add a prefix to indicate what kind of release this pull request corresponds to. For reference, see https://www.conventionalcommits.org/

Available types:
 - feat: A new feature
 - fix: A bug fix
 - docs: Documentation only changes
 - style: Changes that do not affect the meaning of the code (white-space, formatting, missing semi-colons, etc)
 - refactor: A code change that neither fixes a bug nor adds a feature
 - perf: A code change that improves performance
 - test: Adding missing tests or correcting existing tests
 - build: Changes that affect the build system or external dependencies (example scopes: gulp, broccoli, npm)
 - ci: Changes to our CI configuration files and scripts (example scopes: Travis, Circle, BrowserStack, SauceLabs)
 - chore: Other changes that don't modify src or test files
 - revert: Reverts a previous commit

@ctreatma
Copy link
Contributor Author

Note that there will be times when we don't want enums to be flexible, too. For example, we have some oneOf relationships where the nested schemas look like this:

...
  - properties:
    - name: type
       type: string
       enum:
         - vrf
...
  - properties:
    - name: type
       type: string
       enum:
         - public_ipv4
         - public_ipv6

In this case, we need those enums to be evaluated strictly so that a JSON object received from the API does not match more than one schema in the oneOf.

* When an instance is powered on completely it will move to the `active` state.
* Using the reinstall action to install a new OS on the instance will cause the instance state to change to `reinstalling`.
* When the reinstall action is complete the instance will move to `active` state.
$ref: './DeviceStateFlexible.yaml'
Copy link
Member

Choose a reason for hiding this comment

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

We could write a small script to automate this (and upstream those changes and that helper).

@@ -0,0 +1,3 @@
anyOf:
- $ref: './DeviceState.yaml'
- type: string
Copy link
Member

Choose a reason for hiding this comment

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

Oh.. now I get it.

Copy link
Member

Choose a reason for hiding this comment

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

Am I missing something, or do they not generate a way to access the string (non-enum) values?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, no, it looks like the generator doesn't successfully expose the fallback: https://github.com/equinix-labs/metal-go/pull/134/files#diff-ba37d6e9378c96645c1576db6979c216eb53740c1482bc5719d328322ee0cfb4R22

That seems like a generator bug, but an alternative approach might be to create an EnumFallback.yaml schema that is just a string, and reference that instead of putting type: string directly in the anyOf.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now that we're all-in with custom templates, a different option would be something like an x-enum-as-string extension: if a property has x-enum-as-string: true, define the corresponding attribute as a plain old string instead of an enum. I think we'd still want to generate the enum for convenience, though (so there'd still be, e.g., a DeviceStatus enum, but Device.Status would be a string, not a DeviceStatus. We'd need to be careful not to use x-enum-as-string: true on enums on schemas that are in oneOf relations.

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 looks like we could use this anyOf construct alongside the SIMPLIFY_ANYOF_STRING_AND_ENUM_STRING generator option to do what's described in the previous comment, with the same caveat that we need to be careful to only add flexibility where it makes sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

While investigating fixes for #154 I found that the SIMPLIFY_ANYOF_STRING_AND_ENUM_STRING doesn't seem to do anything; I could see from generator output that the option was recognized and accepted by the generator, but it didn't actually simplify the anyOf to a string. More digging is needed to see if this is a generator bug or an issue with the combination of config options we're using.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants