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

Use hashicorp/terraform-registry-address as a decoupled library #28338

Merged
merged 3 commits into from
Jul 8, 2022

Conversation

radeksimko
Copy link
Member

@radeksimko radeksimko commented Apr 12, 2021

This PR proposes replacing some of the internal logic of addrs package which handles parsing and representation of the provider registry address.

As discussed earlier there is now a decoupled library at https://github.com/hashicorp/terraform-registry-address , which effectively commits to that API there and replacing the internals here ensures that no breaking changes are introduced without other external consumers noticing - soon to be Terraform LS, and likely the plugin SDK too.


I originally wanted to replace all imports throughout the codebase but later realized how many there are, so I decided to just leave the existing functions, constants and types in place and just "proxy" it to the library. I'm willing to do a more thorough replacing if desired though.

Also I do plan to tag terraform-registry-address@v1.0.0, but I thought I'd raise this PR first to double check with you all before formally/externally committing to that API.

@codecov
Copy link

codecov bot commented Apr 12, 2021

Codecov Report

Merging #28338 (0f02b08) into main (1212bbe) will decrease coverage by 0.02%.
The diff coverage is 78.94%.

Impacted Files Coverage Δ
addrs/provider.go 64.00% <78.94%> (-1.19%) ⬇️
dag/marshal.go 61.90% <0.00%> (-1.59%) ⬇️
states/statefile/version4.go 58.19% <0.00%> (+0.23%) ⬆️
internal/providercache/dir.go 73.46% <0.00%> (+6.12%) ⬆️

@mildwonkey
Copy link
Contributor

🤔 We already have precedent for more general libraries which are intended to work with multiple versions of terraform - terraform-config-inspect - and when you'd first shared terraform-registry-address I had imagined that it was designed along the same lines, as something decoupled from terraform. All in all, I'd rather we define an API together that core is willing to commit to than to extract this code and import this library.

I know this is a weird one, but the library name gives me pause. These aren't just registry addresses, these are provider source addresses which assume a registry source. It sounds super picky, but I have two concerns based on the same fact: as someone working on terraform or that library, I'd assume it was registry specific, and therefore either not understand when and where to use it, or assume I should make other registry-specific assumptions when adding to the library itself.

Is this library going to continue to be useful for you if we need to make changes that are tightly coupled to terraform releases? Are you planning on extracting more registry-address-specific codepaths out of core into this library? How do we reconcile this with the fact that we tell users not to use terraform as a library?

@hashicorp-cla
Copy link

hashicorp-cla commented Mar 12, 2022

CLA assistant check
All committers have signed the CLA.

@crw
Copy link
Contributor

crw commented Mar 14, 2022

@radeksimko Do you still need this reviewed?

@radeksimko
Copy link
Member Author

I see I missed the questions above 🙈 let me just answer these for posterity.

Is this library going to continue to be useful for you if we need to make changes that are tightly coupled to terraform releases?

That depends on what kind of changes - I imagine TF Core itself would need to maintain some backwards compatibility anyway in this area, so if anything we'd more likely benefit from these changes.

Are you planning on extracting more registry-address-specific codepaths out of core into this library?

We will certainly need to parse module addresses - but that seems like a separate problem - at least the current TF Core codebase seems to make it a separate problem. So if that ever gets extracted, I imagine it would be a separate part of the library or even a separate library, so the consumer makes more upfront choice via e.g. ParseModuleAddr() or ParseProviderAddr(). I think it's unlikely we'd end up with a single method for both.

I don't foresee other changes/needs atm.

How do we reconcile this with the fact that we tell users not to use terraform as a library?

I think that is reconciled very well through the fact that the address is already part of an explicitly consumable JSON API.


@crw I don't insist on a particular shape of this PR but I do think that a wider discussion is needed about the viability and maintenance of https://github.com/hashicorp/terraform-registry-address in some shape or form.

This actually became more important as a result of the formatted address becoming part of JSON output, see https://github.com/hashicorp/terraform-json/pull/50/files

That is - there are and will be - consumers outside of TF Core who need to parse and compare the (provider) addresses, which effectively became part of a stable JSON API but there seems to be hesitance to commit to the stability of the address format. I'd say that treating the address as a stable API is almost inevitable now. I would understand the argument of "stable by accident" vs "stable by intention/design" though and perhaps the need to retrospectively document the design more thoroughly.

So I expect the implementation may differ, but I don't quite foresee how the API or behaviour would differ.


TL; DR - feel free to close this PR for now but I sense we will need a sustainable solution sooner or later.

@crw
Copy link
Contributor

crw commented Mar 16, 2022

Thanks for the new context! I raised your new comment with the Core team.

Copy link
Member

@jbardin jbardin left a comment

Choose a reason for hiding this comment

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

Thanks @radeksimko! Other than the one comment on the default function name, this looks like a straightforward transfer of the internal functionality to the new library.

internal/addrs/provider.go Outdated Show resolved Hide resolved
@github-actions
Copy link
Contributor

github-actions bot commented Jul 8, 2022

Reminder for the merging maintainer: if this is a user-visible change, please update the changelog on the appropriate release branch.

@github-actions
Copy link
Contributor

github-actions bot commented Aug 8, 2022

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 8, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants