feat(node): implement initial support to manage APT repositories

[svengreb]

Summary

This pull request implements initial support for managing APT repositories which is (currently) limited to…

• …adding "standard" repositories to allow to configure it.
• toggling the activation status (enabled/disabled) of any configured repository.

Warning

Note that deleting or modifying a repository in any other way is (sadly) not possible (yet?)!
The limited functionality is due to the (current) capabilities of the Proxmox VE APT repository API itself.

Why are there two resources for one API entity?

Even though an APT repository should be seen as a single API entity, it was required to implement standard repositories as dedicated proxmox_virtual_environment_apt_standard_repository. This is because standard repositories must be configured (added) first to the default source list files because their activation status can be toggled. This is handled by the HTTP PUT request, but the modifying request is POST which would require two calls within the same Terraform execution cycle. I tried to implement it in a single resource and it worked out mostly after some handling some edges cases, but in the end there were still too many situations an edge cases where it might break due to Terraform state drifts between states. In the end the dedicated resources are way cleaner and easier to use without no complexity and conditional attribute juggling for practitioners.

Other "specialties"

Unfortunately the Proxmox VE API responses to HTTP GET requests with four larger arrays which are, more or less, kind of connected to each other, but they also somehow stand on their own. This means that there is a files array that contains the repositories again which again contains all repositories with their metadata of every source file. On the other hand available standard repositories are listed in the standard-repos array, but their activation status is only stored when they have already been added through a PUT request. The infos array is more less useless.

So in order to get the required data and store them in the state the importFromAPI methods of the models must loop through all the deep-nested arrays and act based on specific attributes like a matching file path, comparing it to the activation status and so on.

In the end the implementation is really stable after testing it with all possible conditions and state combinations.

@bpg if you'd like me to create a small data logic flow chart to make it easier to understand some parts of the code let me know. I can make my local notes "shareable" which I created to not loose track of the logic.

What is the way to manage the activation status of a "standard" repository?

Because the two resources are modular and scoped they can be simply combined to manage an APT "standard" repository, e.g. toggling its activation status. The following examples are also included in the documentations.

# This resource ensure that the "no-subscription" standard repository is added to the source list.
# It represents the `PUT` API request.
resource "proxmox_virtual_environment_apt_standard_repository" "example" {
  handle = "no-subscription"
  node   = "pve"
}

# This resource allows to actually modify the activation status of the standard repository as it represents the `POST`.
# Using the values from the dedicated standard repository resource makes sure that Terraform correctly resolves dependency order.
resource "proxmox_virtual_environment_apt_repository" "example" {
  enabled   = true
  file_path = proxmox_virtual_environment_apt_standard_repository.example.file_path
  index     = proxmox_virtual_environment_apt_standard_repository.example.index
  node      = proxmox_virtual_environment_apt_standard_repository.example.node
}

Contributor's Note

• I have added / updated documentation in /docs for any user-facing features or additions.
• I have added / updated acceptance tests in /fwprovider/nodes/apt for any new or updated resources / data sources.
• I have ran make example to verify that the change works as expected.

Community Note

• Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
• Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for pull request followers and do not help prioritize the request

[bpg]

Hi @svengreb!

Thank you so much for your contribution!

I've read through most of the code, but I haven't reviewed the acceptance tests yet. I have a few comments that I hope make sense.

[bpg]

I'm not sure if this is a good change. We have logically separate packages under github.com/bpg/terraform-provider-proxmox/, like the API client and the provider itself. Also, wrapping warnings gives a nudge to think about whether the returned error captures enough context to be useful during debugging.

[bpg]

Yeah... the separator character for composite import IDs is all over the places: we're using ?, :, !, /, or combination of them 🤦🏼
I would be nice to come to some common denominator here. : seems to me like a good choice, WDYT?

[bpg]

I've read a bit further in the code, the actual ID stored in the state is using _ separator, which would add to the confusion if someone decide to inspect the stored state.

[bpg]

I would rather inline all those constants. The legacy code uses attribute name constants extensively, but I found it not super useful.

The constant provides indirect reference in the code, so if value under the constant changes, the places there this value is used through the constant don't need too. But that's the opposite from what we want here!

The schema we define here is very much static. Renaming any of the schema attribute should be done carefully, with necessary ceremonies to ensure backward compatibility.

Then the acceptance tests that validate resource templates against the resource schema and implementation, they should not be automatically changed (via referencing a constant rather than the value) when attribute name changes in the schema. Those tests validates a contract of the published resource schema with the current implementation. So if for some reason an attribute name has changed, I want the test to fail first, then want to se a change in the test / schema to ensure the old resource schema still works with the new resource implementation as well as the new resource schema does (with whatever transformation / deprecation logic as necessary)

Another point is that acceptance tests should also be as much human readable as possible, as they are the main tool for us developers to maintain the code. E.g. here I would need to spend quite a bit of time to understand what the test is doing. I don't see much harm in duplication of the test data (i.e. resource templates) in tests for improving readability.

And lastly "comment" is much shorter than SchemaAttrNameComment :)
In most cases the additional context (i.e. 'schema attribute name') is easily deductible form the surrounding code.

[bpg]

I don't think we need adding a prefix to the ID value. IDs don't need to be globally unique, and they are scoped per resource type in TF.

[bpg]

Not quite sure if I understand purpose of theses transformations. Could you perhaps add some examples in the code comments?

[bpg]

I would suggest to avoid "Terraform" wording in the outputs. The provider supports both Terraform and OpenTofu, so the messages specifically pointing to Terraform could be confusing.

[bpg]

Also, would it make more sense to add the actual reported diagnostic error convDiags to diags instead of swallowing it?

[bpg]

I think function.NewFuncError is misused here. This part of the provider-supplied functions package, returning a specific error implementation.
It also makes the API client code dependent on the hashicorp's code, that I'd prefer to avoid if possible.

I could probably just use the standard golang error wrapping technique, i.e.

var ErrStandardRepoHandleKindIllegal = errors.New("illegal APT standard repository handle kind")

...

	return StandardRepoHandleKindUnknown, fmt.Errorf(
		"parse APT standard repository handle kind %q: %w",
		input, ErrStandardRepoHandleKindIllegal,
	)
}

[svengreb]

Sure, I'll try to go through the review until the end of this week 😄

[bpg]

Okay, I'm going to merge it as-is, don't want to let the code rot.

Will address remaining comments in the follow up PRs.