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

importer-rest-api-specs - support resource generation for resources with a scoped ID segment #3586

Merged
merged 4 commits into from
Jan 9, 2024

Conversation

stephybun
Copy link
Member

Currently we skip resource generation if a candidate resource has a scoped resource ID segment. This PR removes that check and flips the logic for setting the Create/Read/Update Property payload instead of erroring.

These changes are required to be able to generate the resource Chaos Studio Targets. This shouldn't result in changes to the existing Data API definitions.

Copy link

github-actions bot commented Jan 9, 2024

Breaking Changes

No Breaking Changes were found 👍

Copy link

github-actions bot commented Jan 9, 2024

Summary of Changes

No Breaking or Non-Breaking Changes were found 👍

Copy link

github-actions bot commented Jan 9, 2024

New Resource ID Segments containing Static Identifiers

No new Resource ID Segments containing Static Identifiers were identified in the set of changes 🤙.

Copy link
Contributor

@tombuildsstuff tombuildsstuff left a comment

Choose a reason for hiding this comment

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

Given the properties fields are becoming optional rather than required, we should update the operationPayloads object to make these pointers - but if we can update that then this otherwise LGTM 👍

Comment on lines +382 to +383
out.createPropertiesPayload = *createPropsModel
out.createPropertiesModelName = *createPropsModelName
Copy link
Contributor

Choose a reason for hiding this comment

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

As both ModelName and Payload for the Properties object are becoming optional rather than required - we should update the operationPayloads struct to make these pointers to signify that - which'd make this more obvious that these are optional in other parts of the codebase

Copy link
Member Author

Choose a reason for hiding this comment

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

As discussed this change causes unwanted effects in the existing generated resources, details in #3588

Comment on lines +403 to 406
if readPropsModelName != nil || readPropsModel != nil {
out.readPropertiesModelName = *readPropsModelName
out.readPropertiesPayload = *readPropsModel
}
Copy link
Contributor

Choose a reason for hiding this comment

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

(as above)

Copy link
Member Author

Choose a reason for hiding this comment

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

As discussed this change causes unwanted effects in the existing generated resources, details in #3588

Copy link

github-actions bot commented Jan 9, 2024

Breaking Changes

No Breaking Changes were found 👍

Copy link

github-actions bot commented Jan 9, 2024

Summary of Changes

No Breaking or Non-Breaking Changes were found 👍

Copy link

github-actions bot commented Jan 9, 2024

New Resource ID Segments containing Static Identifiers

No new Resource ID Segments containing Static Identifiers were identified in the set of changes 🤙.

Copy link

github-actions bot commented Jan 9, 2024

Breaking Changes

No Breaking Changes were found 👍

Copy link

github-actions bot commented Jan 9, 2024

Summary of Changes

No Breaking or Non-Breaking Changes were found 👍

Copy link

github-actions bot commented Jan 9, 2024

New Resource ID Segments containing Static Identifiers

No new Resource ID Segments containing Static Identifiers were identified in the set of changes 🤙.

@stephybun stephybun merged commit 7f749a7 into main Jan 9, 2024
4 checks passed
@stephybun stephybun deleted the f/support-scopes-resources branch January 9, 2024 12:33
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