Skip to content
This repository has been archived by the owner on Nov 14, 2020. It is now read-only.

Add federation_upstream resource #55

Merged
merged 16 commits into from
Jul 3, 2020

Conversation

niclic
Copy link
Contributor

@niclic niclic commented Apr 30, 2020

Why do we need this PR?

Notes

Most of this new code is the same as the other resources - there are many similarities and I mostly just copied what was already done to be consistent.

Some noteworthy changes.

  • Extracted a common parseResourceId function to util.go.
    The intention it to later refactor other resources to use this function instead of copying it around.

  • Added -count=1 to make testacc to avoid caching tests.
    I think this is a worthwhile change to make.

  • I imported github.com/hashicorp/terraform-plugin-sdk/helper/acctest to create random resource names for test resources. This added quite a few additional vendor dependencies. This is a good practice, but only really needed for running parallel test suites. I think it might be a good idea to do that. However, I can remove the random names from the tests if you prefer this changeset to be smaller.

  • I'm not usually a fan of encoding another APIs default values downstream, but in this case, I did include the documented default values for some attributes.

  • When adding federation support to the rabbithole API, I also added a runtime parameter API, and that is what federation uses internally. After this PR has been approved and merged, I will add support for runtime parameters to this terraform provider, since it can be useful to work with parameters directly in some cases.

@niclic niclic force-pushed the add-federation-resource branch from 80393f1 to 99dfacf Compare May 1, 2020 22:43
@ghost ghost added the dependencies label May 2, 2020
@niclic

This comment has been minimized.

go.mod Outdated Show resolved Hide resolved
@ghost ghost added size/XXL and removed size/XL labels May 6, 2020
@niclic niclic changed the title WIP: Add federation_upstream resource Add federation_upstream resource May 6, 2020
@niclic niclic marked this pull request as ready for review May 7, 2020 17:02
@niclic
Copy link
Contributor Author

niclic commented May 21, 2020

A new release of rabbit-hole has just landed, 2.2.0. I will get this PR fixed-up shortly.

@niclic

This comment has been minimized.

@cyrilgdn cyrilgdn self-requested a review May 26, 2020 06:48
@cyrilgdn
Copy link
Contributor

@niclic Many thanks for your work no this !

I'll take a look as soon as I can. (It may take a while though, it's not a tiny one 😅 and I never used this plugin yet)

@niclic
Copy link
Contributor Author

niclic commented May 26, 2020

Okay, thanks. I'm actually just running through some tests using the actual provider as a final sanity check. Let me get back with the results of those.

As for the module dependency updates that come with the upgrade to the latest version of rabbit-hole, perhaps they should come as a separate PR first?

@niclic niclic mentioned this pull request May 27, 2020
@niclic
Copy link
Contributor Author

niclic commented May 27, 2020

I pulled out the dependency changes into #57.

@cyrilgdn
Copy link
Contributor

I'll take a look at this PR this week (probably on Friday or during the week-end). Thanks again for all your work on this provider 👍

@niclic
Copy link
Contributor Author

niclic commented Jun 15, 2020

Great, thanks. Once #57 is merged, I will finish this. Just want to add a complete example to the docs, including defining a policy for federation. Then it will be done from my end.

@niclic niclic force-pushed the add-federation-resource branch from 14cafad to f03f04d Compare June 17, 2020 17:27
@ghost ghost added size/XL and removed size/XXL labels Jun 17, 2020
@niclic
Copy link
Contributor Author

niclic commented Jun 17, 2020

Okay, this should be complete from my end.

Required quite an involved rebase, to remove all the vendor and mod changes that happened before #57.

Unable to remove 68664e9, and had to add 05d845a to compensate for it. Also, as expected, some commits appear out of order because of the history rewrite.

If this PR gets squashed, then these issues will go away.

@cyrilgdn

Copy link
Contributor

@cyrilgdn cyrilgdn left a comment

Choose a reason for hiding this comment

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

Very nice work 👍 Thanks a lot!

I never used this plugin so I trust you and the tests to assert it works :)

I'll squash in one commit indeed.

I'll let you know here when it's released.

@cyrilgdn cyrilgdn merged commit 329d2b6 into hashicorp:master Jul 3, 2020
@cyrilgdn
Copy link
Contributor

@niclic FYI, this has just been released in v1.4.0.

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.

2 participants