search

equinix / equinix-sdk-go

Golang client for Equinix APIs
https://deploy.equinix.com/labs/equinix-sdk-go
MIT License
2 stars 4 forks source link

build: Remove OAS3 patch artifacts from the repo #55

Open displague opened 2 months ago

displague commented 2 months ago

We've discussed removing the patch artifacts from the repo as they create some review noise. The argument has been that it is sufficient to see the incoming spec changes and any changes to the patches. This PR is a demonstration of that.

(I would link to where this conversation has taken place but I can't find it. Perhaps in equinix-labs/metal-go or fabric-go)

Considerations:

thogarty commented 1 month ago

Hi @displague , right now we have our spec patches being performed on the spec/services/fabricv4/oas3.patched/swagger.yaml file as you can see in all the patches present in this directory: https://github.com/equinix/equinix-sdk-go/tree/main/spec/services/fabricv4/patches I.e. https://github.com/equinix/equinix-sdk-go/blob/ca7c9a77e5a90f57aba25ec9dd7c3994db568260/spec/services/fabricv4/patches/20240305_use_anyType_for_change_operation_value.patch#L1

I'm fine with removing that file, but do you recommend moving these patches to the fetched file? I think the Makefile will need to be updated to reflect that change because currently it is building the SDKs from the patched spec file: https://github.com/equinix/equinix-sdk-go/blob/ca7c9a77e5a90f57aba25ec9dd7c3994db568260/Makefile.fabricv4#L43C1-L53C50

ctreatma commented 1 month ago

My initial thought was to remove the fetch spec. There's some value in keeping that for services like Metal and LBaaS where the spec changes but the spec URL doesn't, but my understanding is that most Equinix API specs have versioned URLs, and we should encourage teams that aren't publishing specs to a place with versioned URLs to align on that for tracking spec diffs across API releases. The spec update process would remain the same: we fetch the spec, overwrite the oas3.patched directory with what was fetched, and apply patches to it. The only difference is that the oas3.fetched directory is removed from git and ignored. I think that approach requires the fewest changes to the overall SDK process for services that are already onboarded.

displague commented 1 month ago

Converted to draft.

In the initial attempt, I was gitignoring all fetched spec files but only modified the Metal Makefile. Fabric and others, including the onboarding, would need to apply whatever change (if any) comes of this exploration.

make patch is cheap for development today because it doesn't depend on fetching the latest spec. It can be used during the creation and testing of spec patches.

I took a look at unifying the fetched and patched directories. In this attempt:

I haven't commit any of those changes because of these points.

I'm open to closing this PR or applying other suggestions. Also open to reasons why the above points are non-blocking or ways to get around those problems 😃

ctreatma commented 1 month ago

The biggest downsides of tying fetch and patch together seem to be:

  1. Fetching adds time to patching
  2. Fetching could bring in unexpected/unwanted changes

The first is only an issue for Metal; historically the spec has required heavy patching in order to be useable, and since we use a git patching mechanism rather than something that modifies the YAML (via an overlay or as an in-memory object), stacking patches for a single large file on top of each other became unwieldy. To resolve this we shipped the spec as multiple files. As a result, fetching the latest Metal spec takes on the order of minutes. In general, Equinix API specs are shipped as a single file, and fetching a single static file shouldn't add that much overhead to the patching process.

The second issue is also currently only an issue for Metal (although if the LBaaS team onboards to this SDK, they would also run into problems because their spec URL does not change with the spec). In my opinion, we have a clear fix for this, which is for those teams to publish their API specs to a place that has versioned URLs.

Based on all of that, it might be better to start with other services (Fabric and/or EIA) for this PR and then the Metal team can address the issues with their spec publishing and patching process when they are able.

@thogarty can you confirm if the EIA spec follows a similar process to the Fabric spec, so that the spec URL changes when a new version is published?

thogarty commented 1 month ago

The biggest downsides of tying fetch and patch together seem to be:

  1. Fetching adds time to patching
  2. Fetching could bring in unexpected/unwanted changes

The first is only an issue for Metal; historically the spec has required heavy patching in order to be useable, and since we use a git patching mechanism rather than something that modifies the YAML (via an overlay or as an in-memory object), stacking patches for a single large file on top of each other became unwieldy. To resolve this we shipped the spec as multiple files. As a result, fetching the latest Metal spec takes on the order of minutes. In general, Equinix API specs are shipped as a single file, and fetching a single static file shouldn't add that much overhead to the patching process.

The second issue is also currently only an issue for Metal (although if the LBaaS team onboards to this SDK, they would also run into problems because their spec URL does not change with the spec). In my opinion, we have a clear fix for this, which is for those teams to publish their API specs to a place that has versioned URLs.

Based on all of that, it might be better to start with other services (Fabric and/or EIA) for this PR and then the Metal team can address the issues with their spec publishing and patching process when they are able.

@thogarty can you confirm if the EIA spec follows a similar process to the Fabric spec, so that the spec URL changes when a new version is published?

Yes, EIA and Fabric have the same process that you mentioned. I'm ok with limiting it to them to start.