-
Notifications
You must be signed in to change notification settings - Fork 3
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
Openapi fix spec #30
Openapi fix spec #30
Conversation
d19528b
to
5eff772
Compare
The code now successfully compiles. Please check if the changes to the spec file are appropriate. |
9945c72
to
92216ee
Compare
if 'failover_strategy' in schema.get('properties', {}): | ||
prop = schema['properties']['failover_strategy'] | ||
if 'default' in prop and prop['default'] is None: | ||
print(f"Removing 'default: null' in {name}.failover_strategy") | ||
del prop['default'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For reference:
failover_strategy:
type: object
properties:
value:
type: string
enum:
- ''
- active-active
- active-passive
label:
type: string
enum:
- (unspecified)
- Active/Active
- Active/Passive
default: null
Looks like the default should probably be the empty string, not null.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also:
failover_strategy:
default: null
oneOf:
- $ref: '#/components/schemas/FailoverStrategyEnum'
- $ref: '#/components/schemas/BlankEnum'
Not immediately clear to me whether the issue is with both or either of these cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per slack it's the latter case that's an issue. Probably both are wrong though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possibly relevant: OpenAPITools/openapi-generator#16775
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not clear yet where the null
is coming from here as both the model and the serializer define blank=True
and allow_blank=True
but not null=True
or allow_null=True
here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this is an issue of the specification, right?
Should I create an issue on nautobot's project?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that would be appropriate, yes. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Created an issue :)
nautobot/nautobot#6157
development/scripts/fix-spec.py
Outdated
# Replace nested `type` field in PowerFeed | ||
if 'type' in schema['properties']: | ||
type_property = schema['properties']['type'] | ||
if 'properties' in type_property and 'value' in type_property['properties']: | ||
value_property = type_property['properties']['value'] | ||
if 'enum' in value_property and set(value_property['enum']) == {'primary', 'redundant'}: | ||
print(f"Replacing complex 'type' field in PowerFeed") | ||
schema['properties']['type'] = { | ||
'type': 'string', | ||
'enum': ['primary', 'redundant'], | ||
'default': 'primary' | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For reference:
type:
type: object
properties:
value:
type: string
enum:
- primary
- redundant
label:
type: string
enum:
- Primary
- Redundant
default:
value: primary
label: Primary
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an intended asymmetry in the API - on responses from the server, type
should be a dict with keys value
and label
, but on requests to the server, the client should just specify a string corresponding to the value
. The PowerFeed
type should only be used on the response side, on the request side see WritablePowerFeedRequest
, PatchedWritablePowerFeedRequest
, BulkWritablePowerFeedRequest
, PatchedBulkWritablePowerFeedRequest
Same comment on the other enum fields below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this something we need to change here then and/or something that needs to be changed in the specification?
How should go-nautobot handle this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure yet. I currently believe that the generated schema is correct but there may be some nuance I'm missing.
development/scripts/fix-spec.py
Outdated
# Replace nested `supply` field in PowerFeed | ||
if 'supply' in schema['properties']: | ||
supply_property = schema['properties']['supply'] | ||
if 'properties' in supply_property and 'value' in supply_property['properties']: | ||
value_property = supply_property['properties']['value'] | ||
if 'enum' in value_property and set(value_property['enum']) == {'ac', 'dc'}: | ||
print(f"Replacing complex 'supply' field in PowerFeed") | ||
schema['properties']['supply'] = { | ||
'type': 'string', | ||
'enum': ['ac', 'dc'], | ||
'default': 'ac' | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For reference:
supply:
type: object
properties:
value:
type: string
enum:
- ac
- dc
label:
type: string
enum:
- AC
- DC
default:
value: ac
label: AC
development/scripts/fix-spec.py
Outdated
if 'phase' in schema['properties']: | ||
phase_property = schema['properties']['phase'] | ||
if 'properties' in phase_property and 'value' in phase_property['properties']: | ||
value_property = phase_property['properties']['value'] | ||
if 'enum' in value_property and set(value_property['enum']) == {'single-phase', 'three-phase'}: | ||
print(f"Replacing complex 'phase' field in PowerFeed") | ||
schema['properties']['phase'] = { | ||
'type': 'string', | ||
'enum': ['single-phase', 'three-phase'], | ||
'default': 'single-phase' | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For reference:
phase:
type: object
properties:
value:
type: string
enum:
- single-phase
- three-phase
label:
type: string
enum:
- Single phase
- Three-phase
default:
value: single-phase
label: Single phase
development/scripts/fix-spec.py
Outdated
# Replace complex `type` object with a simpler string enum in Prefix | ||
if 'type' in schema['properties']: | ||
type_property = schema['properties']['type'] | ||
if 'properties' in type_property and 'value' in type_property['properties']: | ||
print(f"Replacing complex 'type' field in Prefix") | ||
schema['properties']['type'] = { | ||
'type': 'string', | ||
'enum': ['container', 'network', 'pool'], | ||
'default': 'network' | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For reference:
type:
type: object
properties:
value:
type: string
enum:
- container
- network
- pool
label:
type: string
enum:
- Container
- Network
- Pool
default:
value: network
label: Network
53e6ab4
to
ac5af4d
Compare
ac5af4d
to
cae84f3
Compare
cae84f3
to
dcf52af
Compare
@chadell Is there something else that should be changed? :) |
|
||
- [Network Automation with Go Book](https://github.com/PacktPublishing/Network-Automation-with-Go/blob/main/ch06/nautobot/main.go) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that having an example is great to get started, and adding the reference to this book it's good too even though is using an old version
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed the reference to the terraform provider, as that will use the new go-nautobot code :)
Is that alright?
da86890
to
3da61de
Compare
.README.md
Outdated
|
||
func main() { | ||
token := "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" | ||
check(err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
err
is not defined at this point, please remove
.README.md
Outdated
check(err) | ||
|
||
config := nb.NewConfiguration() | ||
config.Servers[0].URL = "localhost" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
demo.nautobot.com
?
3da61de
to
e8af195
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This switches to openapi-codegenerator and tries to patch the spec file with the patcher from netbox. Doesn't solve all issues yet.