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

feat: WIP experimenting with Smartview service interface #86

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

displague
Copy link
Member

@displague displague commented Jan 7, 2025

Inclusion of SmartView APIs in the Equinix Go SDK repo would enable Terraform datasources and resources for interacting with these services.

This PR is intended to demonstrate how the various SmartView APIs (https://developer.equinix.com/catalog) could be merged together and packaged under a single service definition, "smartview".

Some considerations:

  • the package is not versioned (smartview). Specs were combined representing both SmartView /v1 and /v2 endpoints. This could be separated into two different packages to be more equinix-sdk-go conventional. (smartviewv1, smartviewv2)
  • the specs that are used in this PR are from the latest published Swagger (OAS2) specs
  • there is one OAS3 API SmartView spec captured in the API Catalog, "IBX SmartView System Alert APIs". This /v2 spec was excluded for this draft because the combining tools can not merge OAS2 and OAS3 (and I didn't add an OAS2->3 conversion step this go around)
  • curl can not access the spec URLs defined in the Makefile. These files can be fetched via a browser. I'm not sure why curl is not able to fetch these today. I included copies in the repo to make the PR easier to reason about.

The generated API service is documented here: https://github.com/equinix/equinix-sdk-go/blob/a855ffb8e5f867ef1ea11e7f3c5ae79cb0f564c4/services/smartview/README.md
Note:

  • "Mixin" operationIDs are not desirable, they are a result of the combining

Browse the changes by commit to avoid the slowness of exploring all of the generating artifacts.
https://github.com/equinix/equinix-sdk-go/pull/86/commits

Ultimately this WIP may be used to recommend upstream spec definition changes, such as migration to a single OAS3 document. The combined spec file in this PR could assist in that.

id: patch
if: ${{ always() && steps.fetch.conclusion == 'success' }}
run: |
make -f Makefile.smartview patch
Copy link
Member Author

Choose a reason for hiding this comment

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

We'll want to combine before patching. We would want to be careful about the order of the source URLs being combined so that the fetched+combined spec can be patched against.

@@ -10,9 +10,12 @@ USER_AGENT=${GIT_REPO}/${PACKAGE_VERSION}

OPENAPI_IMAGE_TAG=v7.4.0
OPENAPI_IMAGE=openapitools/openapi-generator-cli:${OPENAPI_IMAGE_TAG}
GOSWAGGER_IMAGE_TAG=v0.31.0
GOSWAGGER_IMAGE=ghcr.io/go-swagger/go-swagger:${GOSWAGGER_IMAGE_TAG}
Copy link
Member Author

Choose a reason for hiding this comment

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

go-swagger was the tool I ultimately settled on after trying a number of others. The common failing in other "combine" tools was that they left $ref: spec_1.yaml#/... in the combined spec documents. The go-swagger flatten feature (or attribute of combining) is what was missing in the other tools.

CRI=docker # nerdctl
CRI_COMMAND_BASE=${CRI} run --rm -u ${CURRENT_UID}:${CURRENT_GID} $(DOCKER_EXTRA_ARGS)
OPENAPI_GENERATOR=${CRI_COMMAND_BASE} -v $(CURDIR):/local ${OPENAPI_IMAGE}
GOSWAGGER=${CRI_COMMAND_BASE} -v $(CURDIR):/local -w /local ${GOSWAGGER_IMAGE}
OPENAPI_GENERATOR=${CRI_COMMAND_BASE} -v $(CURDIR):/local -w /local ${OPENAPI_IMAGE}
Copy link
Member Author

Choose a reason for hiding this comment

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

I added -w /local because the argument references sometimes lead with /local and sometimes didn't. As I moved between local tools and docker tools the /local part of the argument list needed to be removed. Working out of /local means we don't need to lead with /local, but it doesn't hurt to have it.

https://developer.equinix.com/sites/default/files/smartview-v1-catalog-alert_v1_0.yaml \
https://developer.equinix.com/sites/default/files/smartview-v1-catalog-assets_v1_1.json \
https://developer.equinix.com/sites/default/files/smartview-v1-catalog-hierarchy_v1_2.json
# TODO(OAS3): https://developer.equinix.com/sites/default/files/smartview-v2-catalog-system-alerts_v2_2.json
Copy link
Member Author

Choose a reason for hiding this comment

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

OAS3 spec was excluded.

rm -f ${SPEC_FETCHED_DIR}/*
$(eval INDEX=0)
for URL in ${SOURCE_URLS}; do \
${SPEC_FETCHER} \
Copy link
Member Author

Choose a reason for hiding this comment

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

For whatever reason, these curl requests are failing. I believe this was a problem we had back in early versions of metal-go that was worked-around by adding a meaningful User-Agent. No such luck this time.

Makefile.smartview Outdated Show resolved Hide resolved
-t /local/${TEMPLATE_DIR} \
-o /local/${CODE_DIR} \
--skip-validate-spec \
--openapi-normalizer FIX_DUPLICATED_OPERATIONID=true \
Copy link
Member Author

Choose a reason for hiding this comment

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

FIX_DUPLICATED_OPERATIONID may not be needed once the duplicated spec_.yaml / spec_2.yaml is removed.

Makefile.smartview Show resolved Hide resolved
# # --merged-spec-filename /local/${MERGED_SPEC_FILE}
${GOSWAGGER} mixin ${SPEC_FETCHED_DIR}/spec* --output=${SPEC_FETCHED_DIR}/merged-spec.yaml --format=yaml --keep-spec-order || true
${GOSWAGGER} flatten --with-flatten=minimal ${SPEC_FETCHED_DIR}/merged-spec.yaml --output=${SPEC_FETCHED_DIR}/${SPEC_ROOT_FILE} --format=yaml
# rm ${SPEC_FETCHED_DIR}/merged-spec.yaml ${SPEC_FETCHED_DIR}/spec*
Copy link
Member Author

Choose a reason for hiding this comment

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

We'll want to remove the source spec files when they can be fetched reliably.

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.

1 participant