-
Notifications
You must be signed in to change notification settings - Fork 2
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
**BREAKING CHANGE** to CT resource schema: Change primitives from set
to map
#998
Merged
chrismarget-j
merged 14 commits into
main
from
993-changing-ct-primitive-elements-has-unintended-downstream-effects
Dec 21, 2024
Merged
**BREAKING CHANGE** to CT resource schema: Change primitives from set
to map
#998
chrismarget-j
merged 14 commits into
main
from
993-changing-ct-primitive-elements-has-unintended-downstream-effects
Dec 21, 2024
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
TODO: - tests - ensure primitive ID survives
chrismarget-j
changed the title
WIP: 993 changing ct primitive elements has unintended downstream effects
**BREAKING CHANGE** CT resource schema - change primitives from set to map
Dec 19, 2024
chrismarget-j
changed the title
**BREAKING CHANGE** CT resource schema - change primitives from set to map
**BREAKING CHANGE** to CT resource schema: Change primitives from Dec 19, 2024
set
to map
Closes #977 |
rajagopalans
approved these changes
Dec 21, 2024
chrismarget-j
deleted the
993-changing-ct-primitive-elements-has-unintended-downstream-effects
branch
December 21, 2024 22:07
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Problem Statement
Connectivity Templates are implemented as trees of
ep-endpoint-policy
nodes. Our previous implementation of CT resources persisted the ID of the root node (the specificep-endpoint-policy
node which represents the whole CT) through edits, but did not persist the IDs of branch and leaf nodes within the tree. Edits to an existing CT modified IDs deep inside the tree. These changes had adverse effects to applied CTs which required follow-on modifications like configuring IP addresses on point-to-point logical links.Schema Change
CT primitives used to be stored as sets, each with a
name
attribute. Now they are maps, keyed by the primitive name.Old Style
New Style
Details
Preventing ID churn deep within the tree of CT primitives requires us to keep track of the IDs of those primitives. In the old style config, primitives were members of sets, and those sets could nest a few layers deep.
Because set elements are dereferenced by their value, we can't put
Computed
attributes (the IDs) into an object which represents a set element. Adding the ID would change the value, destroying the old element, and creating a new one. Terraform would complain.The new scheme solves that problem by keeping the primitives as (nested) maps. Maps elements are referenced by their key, so
Computed
values do not cause a problem.We now keep the pipeline (upstream), policy, and batch (downstream) IDs for each primitive, and re-use these values during
Update()
.Summary of impacted files
It's a big PR. 31 files are modified here. They are:
apstra/resource_connectivity_template_<type>.go
- 4 files - these are the top-level resource files. The only change here is thatRequest()
andUpdate()
now invokeplan.LoadPrimitiveIds()
, which is a new method on the resource model struct. We do this because (a) IDs are generated locally on the client side (a quirk of the CT API), and (b) they're nested deep within the request struct, so teasing them out warranted a dedicated method.apstra/resource_connectivity_template_<type>_integration_test.go
- 4 files - these are the top-level resource test files. The changes here revolve around generating acceptance test checks and the change fromset
tomap
.apstra/blueprint/connectivity_templates/connectivity_template_<type>.go
- 4 files - These are the resource model struct files. The changes here revolve around theset
/map
distinction and the introduction of theLoadPrimitiveIds()
method.apstra/blueprint/connectivity_templates/primitves/<type>.go
- 10 files - These files represent the 10 primitive types which make up the branches and leaves of the CT node tree. The changes in here were driven by themap
/set
schema change, along with the introduction of 3 IDs to the schema for each primitive. Those IDs represent the primitive data, the pipeline (upstream) node, and the batch (downstream) node. Each file has a new functionLoadIDsInto<type>Map()
, which is called by the parentLoadPrimitiveIds()
method.examples/
anddocs/
- 8 files - pretty clear cutapstra/resource_datacenter_connectivity_template_primitives_test.go
- 1 file - This file contains test helper methods for each of the 10 primitives. These helpers are used to generate random configurations during acceptance testing. This thing is probably the biggest diff, because so much stuff got restructured.