The rest of this tutorial assumes you know what Terraform does. If you arrive from a JVM background, you may know it only as “the thing that makes the infra”. That is close enough to get by and not close enough to review a provider. This section is the prerequisite. Five ideas, and no HCL you have to be able to write yourself.

Every plan has exactly three inputs

Almost every bug in the sections ahead is one of these three being wrong, or two of them disagreeing for a reason the provider invented.

A plan, read closely

Two lines matter. Refreshing state is Terraform calling the provider's Read for every resource in state, before it diffs anything. And ~ cost_centre = "eng-platform" -> "eng-network" is the diff: left side from state (as just refreshed), right side from config. By default a plan is that comparison and nothing more: it does not ask the API “is this change legal”. A resource can opt out by implementing ModifyPlan and rewriting the plan itself. Four resources here do, and one of those, the NAT gateway, calls the API at plan time, so on that one resource a plan really does ask. That is the exception. Know it exists.

Which is why -/+ destroy and then create replacement is the line to fear. On this provider a replacement is a real circuit being decommissioned and a new one ordered: minutes of downtime, a new UID, new billing. Terraform will print it calmly in the middle of forty other lines. A plan modifier in the provider decides when that happens.

The config is the whole desired state

Deleting a line from a config is a change to the desired state. It is not “stop managing this field”.

Null, unknown, and known

Null is one of three states an attribute can be in, and the third has no JVM equivalent at all. A framework value is not a nullable Kotlin type with two cases. It has three, and provider code has to handle all of them.

Unknown appears during a plan, for a value nothing has computed yet, and it survives into the apply: Create and Update are handed the planned state with those unknowns still in it, and filling them in is their job. It matters in review because a guard that tests for null and stops there lets unknown straight through, and what reaches the API is the empty string. Go, for JVM Reviewers has the code shape and the failure it produces.

Refresh, and drift

Drift is the real world moving without Terraform. Someone renames a port in the portal, an API-side default changes, a service gets decommissioned by support. Refresh is how Terraform notices: it calls Read, updates state from what the API said, and only then diffs against config. So the contract on Read is strict and slightly counterintuitive:

Why import exists

Import is a one-attribute-wide test of your Read method, and a cheap one. Which is why the acceptance tests in this repo have an ImportState step with ImportStateVerify: true: it imports the resource it just created into a second state entry and asserts the two match attribute by attribute. Anything Read forgets shows up there as a mismatch. A PR that adds an attribute and no ImportState coverage has left that gap open.

Start here, because it sets the value of every other check in this guide. A green checkmark tells you the code compiles, lints, and that the committed docs match the schema. Behavior against a live fabric is a separate gate, and that is the normal arrangement for a Terraform provider: acceptance tests provision real infrastructure and need credentials, so they gate themselves on TF_ACC and are run deliberately rather than on every push. So the useful thing to hold onto is which four jobs run on a pull request, and what each one is evidence for.

The same command, two environments

The unit-test job and an acceptance run are the same go test line. The difference is what is in the environment when it runs.

An acceptance test takes a slot through acquireAccTestSlot(t), and without TF_ACC that helper skips before doing anything else. The slot bounds how many resources provision at once when the test does run. Read the bottom line of the output carefully: a skip is not a failure, so a run that stepped over a test and a run that passed it both end in ok.

One local-only trap: if your checkout sits inside a Go workspace, a go.work alongside a local megaportgo, that command tests against your SDK working copy rather than the version go.mod pins, and depending on the workspace it may not build at all. Prefix it with GOWORK=off to test what CI tests.

The docs gate

The Generate job enforces a rule that is invisible in a diff: everything under docs/ is generated and committed. CI regenerates it and fails if the working tree moved.

What the gate does not prove is that the docs are any good. tfplugindocs pulls example usage from examples/<type>/<name>/, matched on the Terraform type name. A directory whose name does not match the type exactly (singular where the type is plural, for instance) is never picked up at all. The generated page ships with no Example Usage section and the gate still passes, because the empty page the generator produces matches the empty page that is committed. So when a PR adds a resource or data source, open its generated page in the diff and check the example is on it.

Two jobs worth reading precisely

Everything before this is rules. This is the procedure: how far to go on a given PR, what a finding is worth once you have it, and when to stop and ask instead. Read it once, then keep the cheat sheet open for the real thing.

Two passes, and the second one is optional

Decide which pass a PR needs before you start, rather than skimming everything or checking out everything. That decision is the first judgment call, and the run pass has a section of its own.

Block, comment, or let it go

A disagreement about a finding is often a disagreement about which column it belongs in rather than about the code. Name the column and the discussion gets shorter.

One PR, walked through

A constructed PR rather than a real one: an attribute added to a resource, no test, and a description that does not quite match the diff. The steps are the read-pass sequence above, and the second column is what each one returned.

That is a Block, on step 3, because an attribute a user can set that changes nothing is the silently-wrong column rather than the incomplete one. The missing test, the missing generated page and the description mismatch are all real and none of them is the reason to block. Nothing here is a let-it-go. Written up in the template from the PR Review Cheat Sheet:

The order on this page is the order to work in: the four questions first, then the tab matching the shape of the PR, read pass before run pass, then the comment template at the bottom. Ticks are saved in this browser.

Quick Links

You do not need to write Go to review this provider. You need to read it without stopping, and then you need to know which of your instincts to switch off. Both halves are here: first the syntax that makes an experienced JVM engineer pause, in roughly the order you hit it going down a resource file, then the shorter list of habits that are correct in a transactional JVM service and actively wrong in a Terraform provider. Everything shown is real code from internal/provider.

Every file has the same shape

One package per directory, so there is no file-level namespace and no public class line. Visibility comes from capitalization: NewMCRPrefixFilterListResource is exported, mcrPrefixFilterListResource is package private. There is no public, private or protected keyword at all.

Declarations read the other way round

Name first, type second, everywhere: variables, fields, parameters, return values. Once that clicks, most of the unfamiliarity goes away.

Methods, receivers, and the star that matters

func (r *portResource) Create(...) means "method Create on *portResource, with the receiver bound to r". The receiver is this, except you name it yourself and it is one character by convention. The star is not decoration.

Errors are return values, and diagnostics accumulate

There is no throw, no try, no throws clause, and no stack unwinding. A function that can fail returns an extra error value and the caller checks it. So the caller can ignore it, and can carry on after finding one.

The return after an AddError is load-bearing. Omit it and the function keeps running with an error already recorded, usually straight into a nil dereference. Check for the return on every changed error branch.

The reason it is so easy to omit is that resp.Diagnostics.AddError(...) looks like throw and behaves like errors.add(...). It appends to a list on the response and returns normally. Control flow continues to the next statement.

Because there is no central handler, message quality is entirely a per-call-site decision, which makes it a legitimate review comment rather than someone else's problem. The user sees those two strings in their terminal with no stack trace behind them.

There is no null in Go, and three states in Terraform

Every Go value has a zero value and is readable the moment it is declared. Reading never throws, and a deliberate zero is indistinguishable from a value nobody set. Coming from Kotlin nullability or even Java null checks, this is the biggest loss of safety in the language.

On top of that, the framework layer adds a state the JVM has no equivalent for. types.String is not String?. It is null, or unknown, or known, which Terraform, for JVM Engineers introduces. This is what the three states look like in code, and how the guard on them goes wrong.

Both checks in that last guard are load-bearing. A field can be unknown during create and null during update, so !IsNull() && !IsUnknown() is the shape you want to see. A lone !IsNull() lets an unknown through to ValueString(), which is total: it never panics and never signals, so it hands the empty string to the API. A bug report of the form "my optional field was ignored" is this shape.

Absent, or present. There is no null going out

An update DTO in a JVM service usually gets three states for free: field absent, field present and null, field present with a value. Resist mapping that onto an update here. Updates go out as PUT and every field on the request struct carries omitempty, so a nil pointer is dropped and anything non-nil is sent, including a pointer to the zero value. There is no way to spell "send null".

Requests only. Responses keep the third state, so do not carry this one into a Read. The API spec declares nullable fields, and diversityZone is a frequent one: it documents that only RED and BLUEare returned and that no diversity zone is expressed as a null on a field that is still present. A location's latitude and longitude are nullable too. Unmarshalled into a plain string a null becomes "", so the distinction disappears without erroring anywhere.

Note what is not a row. PtrTo("")sends an empty string, not JSON null, and there is no "clear it" case in the general vocabulary. Where clearing is supported it is a sentinel documented on that one field, which you look up rather than infer: on a VXC VLAN, 0 means "allocate one for me" and -1means "untag it". Two fields on the same struct can spell the same intent differently. The struct itself is asymmetric: CostCentre is a *string, but Name is a plain string with omitempty, so setting a name to empty is not expressible at all. The tag drops it.

Go has no address-of for literals, so &0 does not compile. megaport.PtrTo exists to work around that, is used 24 times in the non-test code, and is preferred over a local temporary.

Nothing rolls back

In the services you already work on, one request is one unit of work: an exception unwinds it, and the database is left consistent because the framework guarantees it. The provider has none of that. A single Create makes two or more calls against a live ordering and billing system, and there is no @Transactional, no compensating action, and no cleanup path.

State is on the user's disk and you cannot migrate it

A bad row in a database you own is recoverable: write a migration, run it, done. The equivalent artifact here is a state file in the customer's S3 bucket or on their laptop, which you cannot read, cannot patch, and cannot see. The only remedies are asking the customer to run terraform state rm or shipping a StateUpgrader in a later release.

Interfaces are implicit, so assertions are manual

There is no implements. A type satisfies an interface by having the right methods, decided at the point of use. That is usually pleasant and occasionally dangerous, because the framework tests for optional interfaces at runtime.

The var (_ resource.ResourceWithImportState = &x{}) block at the top of the file is how this repo buys back the compile-time check, and it is the entire reason those lines exist. All 12 resource files and all 9 data source files have one. A new resource that arrives without it is a resource where a typo in a method name is a silent runtime failure, so ask for the block.

Struct tags are not annotations

tfsdk:"mcr_id" looks like @JsonProperty("mcr_id"), but an annotation is a typed declaration the compiler and your IDE understand. A struct tag is a string literal, read by reflection, at runtime, on first use.

Because the check is at runtime, a tag typo on a rarely exercised attribute survives compilation, the linter, and any test that does not hit that code path. Reading the schema and the model side by side is the only real check, and it is quick.

The nine symbols that carry most of this package, and the one genuine syntax trap in the test files, are a collapsible block on the PR Review Cheat Sheet, because that is where you want them: open next to the diff, not in a section you read once.

What you can safely skip

Review comments you will be tempted to write

Every one of these is good advice in a service you own and wrong here. Guess the reason before opening each.

Three booleans, an optional Default, and a slice of plan modifiers per attribute. That is what decides user-visible behavior, and none of it fails loudly. A wrong combination compiles, passes the linter, generates clean docs, and then does something surprising on someone else's eighteen-month-old config. This is the section to read before reviewing any diff that touches a Schema() function.

The four combinations

Read the last column first. It is the one that decides user-visible behavior.

The trap: unknown is not null

Optional plus Computed is the right shape for “defaults to whatever the API says”, and it is used constantly and correctly. The failure mode is what pairs with it. Here is asn on the MCR resource, which is a fair example of the shape rather than an outlier:

What UseStateForUnknown claims

It says: during plan, if this Computed attribute's value is unknown, use the value from prior state instead of showing (known after apply). Its purpose is cosmetic and real, keeping plans readable by not marking forty computed attributes unknown on every trivial change. Three things it deliberately does not do. It does nothing on create, where there is no prior state to copy, which is why the trap above only fires on update. It does nothing when the config value is itself unknown, because another resource is still to be applied. And a null in prior state is a value like any other, so it will copy that too.

RequiresReplace, and what it costs here

One line, and on this provider it is the highest-consequence line anyone can add to a schema. There is no cloud-style rebuild-in-place. A replacement is a real circuit decommissioned and a new one ordered: minutes to tens of minutes of downtime, a new UID that every dependent VXC references, a new contract term, and new billing.

ModifyPlan, the escape hatch

When per-attribute modifiers cannot express the rule, a resource implements ModifyPlan and rewrites the plan itself. Four resources here do, and each solves a different problem:

The framework calls ModifyPlan on every plan, including the destroy plan, where the plan is null and the state is not, and the create plan, where it is the other way around. All four here open by returning early on those, and the excerpt below starts just after that guard. On a new one it is the first thing to look for, because a body that reads the plan into a model without it also runs on a destroy.

Two review hazards visible in that snippet. A replace rule stated in the schema's plan modifiers and restated in ModifyPlan is one rule in two places, so a PR that adds a RequiresReplace() to a resource that has a ModifyPlan needs the plan hook read as well as the schema. And ModifyPlan is calling the API, which means plan is now doing network I/O. The fail-open branch is deliberate and correct, because a transient lookup failure must not block every plan in every pipeline. A PR that turns that into a hard error is trading a rare wrong plan for a common outage.

Read single_port_resource.go once and you can navigate all twelve resources, because they are built from the same set of framework methods: Metadata, Schema, Create, Read, Update, Delete, Configure and ImportState. Those belong to the Terraform Plugin Framework rather than to this repo: the provider is built on the framework, not the older SDKv2, which is why a resource here is a struct with lifecycle methods hanging off it and why the schema gets a section of its own.

The order is nearly fixed. Learn the exceptions, so you do not conclude a method is missing when it is only somewhere else. Metadata and Schema open every file, and Create, Read, Update, Delete always appear in that order. That is file layout and not execution order: Configure runs before any of them, and a single apply calls exactly one of Create, Update or Delete. In the file, Configure is the one that moves: it follows Delete in six resources, precedes Create in five, and comes last of all in ix_resource.go. VXC, MVE, LAG port and NAT gateway add a ninth method, ModifyPlan. Metadata and Configure are a few lines each, so below are the model struct the file maps onto, then Create and Read expanded, then the remaining three in a table.

The other three, in one table

Review Quizzes

Two scenarios, both built on shapes that occur in this repo. One is a defect. The other is a review comment you should decline.

Two repositories. The provider holds schemas and CRUD methods. The megaportgo SDK holds the HTTP client, the API types and the provisioning polling. The SDK is also used by the Megaport CLI, so a change there has two consumers. Click the layers to expand them.

Every resource file follows the same shape: model struct, then Metadata(), Schema(), Create(), Read(), Update(), Delete(), Configure() and ImportState(), with Configure() turning up in a couple of different positions and four resources carrying a ModifyPlan() as well. Once you have read one you can navigate all of them, which is what Anatomy of a Resource File is for.

That is the entire public surface: twelve resources and nine data sources. A PR adding either has to register it here, and a new resource file with no line in this list is dead code that still passes CI.

Enough domain knowledge to read a diff, and no more. Megaport sells software-defined connectivity: physical ports in data centers, virtual routers, and layer 2 circuits between them. Click a product in the topology to see where it sits. Keep the table open while you review.

Product to resource, and what to check

The only hierarchy that matters for review: a Port, MCR or MVE is an endpoint, and a VXC is a circuit between two endpoints. Endpoints are mostly independent resources with a flat schema. The VXC carries the nesting and the partner configs, so it has a section of its own.

There are three copies of the truth in play: the config the user wrote, the state file Terraform keeps, and what exists in the Megaport fabric. Every method in a resource is an attempt to keep those three aligned, and most bug reports on this provider trace back to those three drifting apart.

The most common bug report

"Provider produced inconsistent result after apply" is the most common bug report on this repo. The message is more useful than it looks: it names the exact attribute and tells you which method to open.

Terraform is not reporting an API failure. It is reporting a contract violation: during plan it recorded a value for vendor_config, the apply succeeded, and then the provider wrote something different into state. Terraform will not accept that, because if it did, the next plan would show a diff the user never asked for. The last line is Terraform's standard wording for this class of error, whatever the underlying cause turns out to be.

Attributes the API never returns have to be carried by hand

vendor_config on an MVE is the clearest example in the repo. It is sent on create, never sent on modify, and never returned by any read. So nothing in the API can tell the provider what it is, and every method that writes state has to source it from the plan or the prior state instead.

That one field is reconciled in three separate places in mve_resource.go: once in Update, and twice more inside ModifyPlan, which has to handle null-in-state, null-in-plan, and case-insensitive comparisons against size and vendor. A carry-forward is a rule spread across every method that writes state, so when a PR changes one of them, find the others.

The framework has a flag with a similar name and the opposite instruction. WriteOnly: true says the value must never be persisted at all: the framework nulls it out of the plan and out of refreshed state, so a hand-written carry-forward line is exactly the wrong fix. Two attributes in the repo declare it, admin_password inside an MVE's vendor_config and pre_shared_keyinside a VXC's ip_sec_tunnel_options. The second is Required, so it has to be in the config on every apply and is null in state in between, which is the intended behavior and not drift.

Import: whatever Read cannot recover is null

All 12 resources implement ImportState. Seven are a single call to ImportStatePassthroughID, which copies the import ID into one attribute and stops. Terraform then calls Read, and Read is what has to reconstruct the entire resource from nothing but that one identifier.

ImportStateVerifyIgnore is where an import test says “except”

The acceptance tests do check the import round trip, then exempt named attributes from the comparison. That exemption list is the one place a test states, out loud, what it is not comparing.

The bottom row is uncontroversial: those genuinely change on their own or are set by the provider itself. Ask two questions about anything else on such a list. Is this an attribute the API cannot return, in which case the exemption is honest? Or is it a round-trip gap that could be closed, in which case the exemption is hiding it? An exemption written by index, such as entries.0.foo and entries.1.foo, is worth a second look on its own, because it only covers as many elements as someone bothered to enumerate. So when a PR adds a name to one of these lists, ask which of the two it is.

Composite IDs, and how short ImportState should be

Child resources need two identifiers, so their import ID is parent_uid:child_id and they cannot use passthrough. They parse the ID themselves. This is the whole job:

Terraform calls Read straight after ImportState, so anything a longer ImportState fetches is fetched again moments later, and its state write is overwritten by what Read derives. Two code paths that read the same object have to keep agreeing with each other, in particular about what a 404 means: erroring in one and removing the resource from state in the other are different outcomes for the same fabric. So when you see a long ImportState, ask what it is doing that Read is not about to do anyway, and whether a value it stamps into state, a timestamp for instance, is one an import should invent.

Drift, and the exit Read must provide

If someone deletes a port in the Megaport Portal, the next Read has to notice and remove it from state without raising an error. Returning an error instead leaves the user wedged: they cannot apply, and they cannot destroy, because both start with a refresh that fails. Every resource calls RemoveResource somewhere in its Read for exactly this reason.

Three distinct triggers, and all three are needed. A deleted product usually returns 404, but sometimes returns 400 with the message Could not find a service with UID, so the status-code test needs two arms. A decommissioned product still reads successfully and reports STATUS_DECOMMISSIONED, so a resource that only inspects the status code keeps a dead service in state forever. The most complete version of all three is the Read method in Anatomy of a Resource File. When a PR touches a Read, compare its error branch against that, rather than against the file next to it.

The VXC (Virtual Cross Connect) resource is the most complex in the provider at ~2,950 lines, with another ~1,600 lines split across vxc_schemas.go and vxc_resource_utils.go, and ~5,000 lines of acceptance tests. It has A-End/B-End configuration, seven partner config variants, VLAN handling semantics, and partner port UID rotation.

Start by counting the ends

A VXC has two ends, and almost every code path that touches one has a near-identical twin that touches the other. The twin is a separate block of code, not a shared function, so a change applied to one end compiles, passes lint, passes CI, and is half-finished.

So a single new end attribute lands in ten places, and only two of them are shared. Treat ten as a floor rather than a total: an attribute with its own update rules picks up more, and inner_vlan also appears in the block after Update that checks the API applied the value it was sent. The partner config schemas are shared (they live as package-level vars in vxc_schemas.go and are referenced from both ends), but the code that reads them is not. Count the switch arms:

Four switch statements, twenty arms, no two of them the same. A PR adding a field to google_config has two Create arms and zero Update arms to worry about; one adding a field to vrouter_config has four. The Update switches are narrow because CSP partner configs are not mutable in place: they are set at order time, and changing them means a new circuit.

VXC Schema Structure

End Configuration Model

Four of those eleven fields come in requested-versus-actual pairs, and the pairs are where to look when a VXC drifts.

With ordered_vlan = 0 the API allocates, say, 2517. Diffing plan-ordered against state-actual then says 0 != 2517 forever, so every unrelated apply, a tag edit, a rate limit change, queues a VLAN mutation on a live circuit. The fix is to compare like with like: plan.OrderedVLAN against state.OrderedVLAN. When you see a diff add a comparison on any Computed field, this is the question to ask.

The supportVLANUpdates() guard on the same condition has its own rule: AWS and Transit connections reject VLAN changes outright, so the provider declines to send them. A PR that adds VLAN mutation to a new partner type has to decide which side of that function it belongs on.

Read cannot round-trip a VXC

Most resources in this provider have a Read that rebuilds state entirely from the API response. VXC cannot, because the fields in the left column above are user intent that the API never echoes back. So fromAPIVXC takes the plan as a fourth argument and copies those fields across by hand.

Two consequences for review. First, a new user-only attribute has to be added to the preservation logic or it silently vanishes from state on the next apply, which looks exactly like drift and is not. Second, Read passes nil because at refresh time there is no plan to preserve from, so importing a VXC leaves those attributes null.

That second point has a useful side effect: the acceptance test's ImportStateVerifyIgnore list is a maintained inventory of exactly which VXC attributes the API does not return.

If a PR adds an entry to that list, ask why. Sometimes the answer is correct, that the API genuinely does not return it. Sometimes it is the quickest way to silence a failing import test, and the real defect is a field that Read should have populated and does not.

This guide does not list what each partner config contains, because that documentation is generated from the schema and so cannot drift: read docs/resources/megaport_vxc.md on the branch, and examples/resources/megaport_vxc/ for a working config of each shape. The part to learn is which end each config belongs on, because both ends share one Go struct and so the schema offers you fields on an end where they do nothing.

Partner config: A-End vs B-End asymmetry

A-End and B-End are just labels for the two endpoints of a VXC. The A-Endis "where the connection starts" and must always be a Port, MCR, or MVE you own (your side); the B-Endis "the other side" and can be a Port, MCR, MVE, Internet Exchange, Marketplace partner, or cloud onramp. For a private Port-to-Port VXC they're effectively symmetric labels. The asymmetry appears when the far side is a CSP, IX, or partner port: those sit only on the B-End, never the A-End.

The a_end and b_end endpoint blocks are genuinely symmetric: same Go struct, same fields. The *_partner_config blocks look symmetric (both reuse vxcPartnerConfigurationModel, so Terraform exposes the same nested fields on each), but they aren't. The partner enum differs, and one value is a deprecated A-End-only legacy.

The MCR-side virtual-router config block has been renamed. The legacy name described where it attaches. The current one describes what it configures. Both still parse, which is the part that matters on review:

Note what the rename did not do. Both blocks carry all seven config attributes, including both vrouter_config and the deprecated partner_a_end_config, because each block references the same shared schema definitions. The only trace the rename left in the schema is the row above: "a-end" is in the A-End partner enum and not the B-End one. Use the current name in new code, and treat the deprecated one as something you will only meet in an existing config.

Because both ends share the underlying struct, Terraform's schema also accepts b_end_partner_config.partner_a_end_config = {…}, but it's effectively dead. The B-End validator rejects partner = "a-end", and the B-End dispatch in Create switches on aws, azure, google, oracle, ibm, transit, and vrouter with no a-end case at all. Setting just the nested block does nothing. The Update path is narrower still: it only rebuilds B-End partner config for transit and vrouter, since CSP partner configs are not mutable in place.

3.8 million downloads. That is the number to hold in your head, because it is the thing that separates reviewing this repo from reviewing an internal service. There is no coordinated rollout, no deploy you can roll back, and no way to see the configs you are about to break. They are on laptops and in pipelines you will never have access to, and the first you hear about a mistake is an issue filed by someone whose Friday you ruined.

Version constraints do not pin users

The version constraint the documentation tells users to use accepts every future 1.x release. So “we will note it in the changelog” is not a mitigation, and neither is “it is only a minor version”. If a change would break a config that works today, it breaks it on an upgrade nobody deliberately chose.

What counts as breaking

Most of these do not look like breaking changes in a diff. Several look like improvements.

The rename pattern, as this repo does it

There are two live deprecations to copy from. partner_a_end_config was superseded by vrouter_config, and MCR's inline prefix_filter_lists was superseded by the standalone megaport_mcr_prefix_filter_list resource. Both kept the old surface working.

Note the cost, because it is the reason a reviewer should push back on gratuitous renames. Two attribute names now describe the same thing, both are wired through the model and the request builder, and a comment in the code has to warn that the two schemas must stay identical. Every future change to that shape has to be made twice. A rename is not free just because it is backward compatible, so “the new name is clearer” needs to be worth carrying both until the next major version.

Where a release note comes from

Asking for “a release note” only helps if you know what produces one, and in this repo it is not a file. There is no CHANGELOG.md and no .changelog/ directory to add an entry to. Pushing a v*tag runs GoReleaser, which uses GitHub's own release-note generator. It groups the merged PRs by their label: feature, enhancement, bug, documentation, and everything else into “Other Changes”.

Two consequences for a reviewer. The PR title isthe release note. A title like “tidy up validators” on a change that breaks existing configs is the whole problem, and asking for a better title is a substantive review comment rather than a nitpick. There is no group for a breaking change, so nothing about the tooling will make one stand out. If a change needs users to notice before they upgrade, the title has to carry it. Someone also has to say so in the release itself, which GoReleaser drafts rather than publishes, so it can still be edited.

SDK version bumps are provider changes

The provider depends on github.com/megaport/megaportgo and shares it with the CLI. A one-line bump in go.mod is a diff with no visible behavior. Three things it can carry:

So the useful review question on a bump is not “does it build”. It is: read the SDK's changelog between the two versions, and for anything touching errors or request shapes, ask for an acceptance test run. A bump is the one change where compiling proves the least, because the whole category of risk is behavior that compiles.

Nine of the provider's registered types are data sources, not resources: read-only lookups that find a location ID, list every VXC in the account, or enumerate the MVE images a vendor publishes. Reviewing one is a smaller job than reviewing a resource, and the differences decide what to check.

Four methods, one of which does the work

Terraform re-reads every data source on every plan and throws the previous result away. There is no prior state to diff, so there is nothing for a plan modifier to modify and no drift to detect. That is why datasource.ReadRequest exposes Config and nothing else: no Plan, no State. The framework draws the same line in the schema types. The attribute structs under datasource/schema have no PlanModifiers field at all, so a PR that tries to add RequiresReplace() or UseStateForUnknown() to a data source attribute does not compile. This is the rare case where the build catches the mistake for you. Know it precisely, so you do not spend a review looking for a no-op that cannot exist.

Validation lives in Read, though it does not have to

site_code in that guard shows a second pattern. It is still a documented attribute, but the v3 locations API dropped it, so using it as your only filter fails at plan time with a written explanation. That is deliberate: keeping the attribute avoids breaking configs that merely reference it, and the error is what stops anyone relying on it. If a PR deletes an attribute outright where this would do, say so.

Two habits that catch the rest

Open the siblings. The three list data sources were written to one design, so a divergence does not stand out. megaport_mves and megaport_vxcs put resource-tag fetching behind an include_resource_tags flag defaulting to false; megaport_mcrs fetches tags unconditionally, one extra API call per MCR, with no way to turn it off. Neither is wrong on its own, and reading one file cannot tell you that.

Grep for the TypeName, not the filename. partner_port_data_source.go is megaport_partner, and singular filenames produce plural Terraform types (mve_image_data_source.go gives megaport_mve_images, and mcr_prefix_filter_list_data_source.go gives megaport_mcr_prefix_filter_lists). Searching for the Terraform type name will not find the Go file.

That last item is cheap to ask for. A data source is a pure function from config to state, so it tests against a fake client in milliseconds, with no Terraform CLI and no API involved. Several here are tested that way, and it is a reasonable thing to expect of a new one.

Terraform provider tests are acceptance tests: they run against a real staging environment, creating and destroying actual resources. That costs time and credentials, so they run deliberately rather than on every push (How to Read the CI Checks has the detail). Read this one with that in mind. The job is to judge whether a PR's new test would catch anything when it runs, and to know what to ask for when it would not.

Which shape does this change need?

Three shapes exist here, and only the first two run on every push. The useful review comment about tests is often not “add a test”. It is that the change was tested in the expensive shape when a cheap one covers it.

Do not skip the middle row. A mock is a struct with the service interface's methods on it, returning whatever the test needs, and it is the only way to cover a 500, a timeout, or a wrapped error in something that runs on every push. If a diff adds an error branch and covers it only in a TestAcc, ask for it in a mocked unit test as well, so every push covers it.

Annotated Test Example

The Check lists are trimmed here. The real test asserts on every computed attribute it can, including product_id, create_date, created_by, location_id, and company_uid.

Running a PR locally takes about ten minutes the first time and about two minutes after that. Given the above, it is the one check that reading the diff cannot substitute for.

One-time setup

Terraform normally downloads providers from the registry. A dev_overrides block tells it to use a local binary instead, so your go install output is what runs. The provider README documents this; the block goes in ~/.terraformrc, with <PATH> replaced by the output of go env GOBIN.

Set these in a dedicated shell, not your profile. The failure mode you are protecting against is a stale MEGAPORT_ENVIRONMENT=production in a shell you forgot about while you are testing a destroy path.

Something to run it against

You need a small Terraform config per resource type, and the provider repo already has them. Use examples/. The first two groups below are the ones you want: each carries its own provider block pointed at staging and provisions real products, and several are staged across numbered files so you can drive an update or a replace rather than just a create. Copy one somewhere outside the repo and it becomes your scratch space. Despite the name, multicloud-scenarios/ is not one of them.

Not everything has a runnable directory. NAT gateway, IX, service keys and the MCR IPsec add-on exist only as resources/<type>/resource.tf snippets, so for those, copy the snippet and add a provider block yourself.

The per-PR loop

A worktree rather than a checkout is worth the extra line. You keep your own branch intact, your .tf files stay put between PRs, and go install from the worktree overwrites the same binary, so switching between two PRs is one cd and one go install.

Three checks, in order of value

The first two are cheap and mechanical, and you should do them on every PR that touches a resource. The third takes thought, and it finds the bugs the first two cannot.

Why zero is the value to try first

Go's zero values meet JSON's omitempty and the result is a field that vanishes. A tag like json:"threshold,omitempty" on an int cannot express zero: the encoder drops the key, so a value the user wrote never reaches the API and nothing errors anywhere along the way. Whether zero is a legal value for that field is a question about the API, and the Go type is free to disagree with it.

You cannot see this in a provider diff, because the tag lives in the SDK repository rather than the one you are reading, and the value is legal so no validator objects. What you can establish in two minutes is whether the value survives an apply and a re-plan. So whenever a PR adds a numeric or optional attribute, the question to ask is: is zero legal for this field, and if it is, does it survive?

If you cannot run it

Sometimes you will not have credentials yet, or the PR needs a cloud account on the far side that you do not have. Two thirds of the value is still available without an apply, so do that part and be explicit about the rest.

Then say so in the review. "Read the diff, ran the unit tests, did not apply this against staging" is a useful review. An approval that reads as if you did apply it, when you did not, is worse than no approval, because the next person assumes the check happened.

Reference, not reading. Do not try to absorb the page in one sitting. Search it when a PR touches an area you have not seen before, and skim the titles once now so you recognize the shapes later. Each entry is a behavior that is non-obvious from the diff, plus what to check when a change lands near it.

17 of 17 shown