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

New resource 'azuread_application_password' #71

Merged
merged 18 commits into from
May 26, 2019
Merged

Conversation

twendt
Copy link
Contributor

@twendt twendt commented Apr 3, 2019

This adds a new resource to manage applications passwords. It is based on the service principal password resource.

@twendt twendt changed the title New resource 'azuread_application_key' New resource 'azuread_application_password' Apr 3, 2019
Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

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

Thank you for the PR @twendt,

Looking at this it seems fairly identical to the existing resource. Would it perhaps make sense to rename it to azuread_object_credential so it can be used for both service principles/application & any others that would work with the API?

@twendt
Copy link
Contributor Author

twendt commented Apr 18, 2019

@katbyte I agree that it would make sense. I looked into the code to see how it can be achieved and I am not sure how to handle the different clients that are required. I was thinking about defining an interface, but the Get function returns return a concrete type of an application or service principal so I guess that won't work.
What is the best approach for this? Should I create a little wrapper type for this?

The other question is about the input. Should we use different inputs for the application id and service principal id and use that to figure out what context we are running in? Or should we just accept an object id and the resource then figures out if the id belongs to an application or service principal?

@ghost ghost removed the waiting-response label Apr 18, 2019
@twendt
Copy link
Contributor Author

twendt commented May 7, 2019

I updated it now so that only a single object_password resource is used for applications and service principals.
I create a little wrapper type that handles the different clients.

The locking is an issue at the moment though. I need to update it to figure the resource_name out that the object_id belongs to to make sure that I lock the application or service principal instead of the object_password resource.

@twendt
Copy link
Contributor Author

twendt commented May 7, 2019

I left the service_principal_password in as it would otherwise break existing installations that use it.

@twendt twendt changed the title New resource 'azuread_application_password' New resource 'azuread_object_password' May 8, 2019
@katbyte katbyte changed the title New resource 'azuread_object_password' New resource 'azuread_application_password' May 25, 2019
@katbyte katbyte added this to the v0.4.0 milestone May 25, 2019
@katbyte
Copy link
Collaborator

katbyte commented May 25, 2019

👋 @twendt,

After some internal discussion we decided to change the approach to one that is a little less general. I hope you don't mind but i have pushed some changed to this effect so we could get this into the next release. Mainly it keeps the schema and client calls separate while moving the shared graph logic and schema out into a helper.

@ghost ghost added size/XXL and removed size/XL labels May 26, 2019
@twendt
Copy link
Contributor Author

twendt commented May 26, 2019

@katbyte Thank you very much for helping me out here.
I really like the new approach, because it is not a breaking change for users that already used the service principal password resource before and I also think that this is more intuitive for the end user.

@katbyte katbyte merged commit 237d8b8 into hashicorp:master May 26, 2019
katbyte added a commit that referenced this pull request May 26, 2019
@ghost
Copy link

ghost commented Jun 26, 2019

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 hashibot-feedback@hashicorp.com. Thanks!

@ghost ghost locked and limited conversation to collaborators Jun 26, 2019
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.

None yet

2 participants