This allows the management of identity entities and aliases of entities #247
Conversation
ghost
added
the
|
Entity related tests pass if I bump the vault version to 0.11.5 GCP Tests pass if I include #243 and bump the vault version to 1.0.0-beta2 |
There was a problem hiding this comment.
@OperationalDev thank you for working on this!
The code looks very good, and the tests as well. It would be great to get some clean tests on this before merging it. I do see that the tests failures appear unrelated and apply to GCP. Would it be possible to merge in master and push it, just to make sure the tests are clean?
docker-compose.yaml
Outdated
| @@ -2,7 +2,7 @@ version: "2.2" | |||
|
|
|||
| services: | |||
| vault: | |||
| image: vault:0.11.1 | |||
| image: vault:0.11.5 | |||
There was a problem hiding this comment.
Would it be possible to add this feature without bumping the Vault version? Or is this absolutely necessary in this PR? I ask because if it is necessary, because I'm following semver in this repo, I'll need to thoughtfully time merging and releasing this PR, which may delay it.
I saw your reference to the alias issue that was fixed in the later versions, so I see why you're doing it. Just checking in about the trade-offs.
There was a problem hiding this comment.
Thanks for taking the time to review my PR.
The only issue with staying on 0.11.1 is that then we cannot move an alias between entities. I don't think this is a big problem.
If we remove the test for moving an alias between entities, then the build does pass (after merging master)
Works for me as I have no intention of moving aliases between entities.
Let me know how you would like to proceed.
There was a problem hiding this comment.
Ok, removed the test and we're looking good on v0.11.1
|
Silly question, can we also submit pull requests for the docs? |
Yes! Docs changes can either be included with the PR that has related code changes, or done separately. |
|
|
||
| if disabled, ok := d.GetOk("disabled"); ok { | ||
| data["disabled"] = disabled | ||
| } |
There was a problem hiding this comment.
Not a blocker at all, but if you feel like it, updating an entity's name is also supported.
There was a problem hiding this comment.
Added updating the entity name
vault/resource_identity_entity.go
Outdated
| return nil | ||
| } | ||
|
|
||
| for _, k := range []string{"name", "metadata", "disabled"} { |
There was a problem hiding this comment.
Just curious about why policies isn't included here.
There was a problem hiding this comment.
No good reason, will add.
| "github.com/hashicorp/vault/api" | ||
| ) | ||
|
|
||
| func TestAccIdentityEntityAlias(t *testing.T) { |
There was a problem hiding this comment.
The tests won't build for me currently due to a dependency issue; I think master needs to be merged in.
There was a problem hiding this comment.
I thought I had merged master in, will take a look.
There was a problem hiding this comment.
Merged in master, should be working now, I managed to build.
OperationalDev
force-pushed
the
add-entities-aliases
branch
from
8439a98
to
87126cb
Compare
|
Made recommended changes. For the docs, I will submit a separate pull request. |
…ases This allows the management of identity entities and aliases of entities
Implementation of feature request #174
Many thanks to @simonswine for all the groundwork on #220