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

added support for identity attribute on container groups #3243

Merged
merged 8 commits into from
Apr 18, 2019
Merged

added support for identity attribute on container groups #3243

merged 8 commits into from
Apr 18, 2019

Conversation

jplane
Copy link
Contributor

@jplane jplane commented Apr 14, 2019

Addresses this enhancement: #3241

Fixes #3241

Copy link
Contributor

@tombuildsstuff tombuildsstuff left a comment

Choose a reason for hiding this comment

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

hey @jplane

Thanks for this PR :)

Taking a look through this mostly LGTM - if we can fix up the minor comments we should be able to run to run the tests and get this merged 👍

Thanks!

azurerm/resource_arm_container_group.go Outdated Show resolved Hide resolved
azurerm/resource_arm_container_group_test.go Show resolved Hide resolved
website/docs/r/container_group.html.markdown Show resolved Hide resolved
@tombuildsstuff tombuildsstuff modified the milestones: v1.26.0, v1.25.0 Apr 16, 2019
Copy link
Contributor

@tombuildsstuff tombuildsstuff left a comment

Choose a reason for hiding this comment

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

hey @jplane

Thanks for pushing those changes - I've run the tests but have noticed a crash when the identity block isn't specified; if we can add a nil-check this should otherwise be good 👍

Thanks!

azurerm/resource_arm_container_group.go Show resolved Hide resolved
@tombuildsstuff tombuildsstuff modified the milestones: v1.25.0, v1.26.0 Apr 16, 2019
tombuildsstuff and others added 2 commits April 16, 2019 16:36
Good catch, sorry for that... in case its not obvious, I'm a bit of a Go newb.  :-)

Co-Authored-By: jplane <[email protected]>
Copy link
Contributor

@tombuildsstuff tombuildsstuff left a comment

Choose a reason for hiding this comment

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

Thanks for pushing those changes @jplane this now LGTM 👍

@tombuildsstuff tombuildsstuff modified the milestones: v1.27.0, v1.25.1 Apr 18, 2019
@tombuildsstuff
Copy link
Contributor

@jplane unrelated to this PR - but it appears that the Windows Server docker images have been relocated from the Docker Hub to Microsoft's Container Registry; as such I'm going to push a commit to make those tests green, I hope you don't mind :)

@tombuildsstuff
Copy link
Contributor

Ignoring the unrelated Windows test failures (mentioned above) the tests pass:

Screenshot 2019-04-18 at 12 06 53

@tombuildsstuff
Copy link
Contributor

reverted the Windows container changes because this is a more involved/unrelated job

@tombuildsstuff tombuildsstuff merged commit eae51bf into hashicorp:master Apr 18, 2019
tombuildsstuff added a commit that referenced this pull request Apr 18, 2019
@jplane jplane deleted the aci-identity branch April 19, 2019 11:18
@ghost
Copy link

ghost commented May 18, 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 👉 [email protected]. Thanks!

@ghost ghost locked and limited conversation to collaborators May 18, 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.

Container Instance: Enable identity config
3 participants