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

Gsuite group name output in gsuite modules #288

Merged

Conversation

ideasculptor
Copy link

@ideasculptor ideasculptor commented Oct 17, 2019

Fixes #287

Sam Gendler added 2 commits October 17, 2019 10:34
of project modules that all use the same group, with only one
creating it.
Copy link
Contributor

@morgante morgante left a comment

Choose a reason for hiding this comment

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

Just one nit.

@@ -36,6 +36,11 @@ output "group_email" {
description = "The email of the created GSuite group with group_name"
}

output "group_name" {
value = module.gsuite_group.name
description = "The email name of the created GSuite group with group_name"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
description = "The email name of the created GSuite group with group_name"
description = "The email of the G Suite group created with group_name"

Copy link
Author

Choose a reason for hiding this comment

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

I didn't quite use your suggestion, as the GSuite spelling wasn't consistent with the prior output, and your description was maybe confusing about the difference between group_email and group_name, so I went with "The group_name of the GSuite group"

Copy link
Contributor

Choose a reason for hiding this comment

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

We should still fix it to be "G Suite" as that is the official name.

If this output is "my-team" (not [email protected]) then the proposed description makes sense.

Copy link
Author

Choose a reason for hiding this comment

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

Yes, that's correct. group_email is the full address, group_name is just the username portion of the address, but I wanted to avoid the word username. I just pushed an update with the spelling correction, too.

@ideasculptor
Copy link
Author

is the failing trigger just that it isn't approved by a reviewer yet, or is something else broken? I cant see the cloud-build details to confirm what is breaking. All of the commits that followed the first one have a failed trigger, but they are only changes to variable and output descriptions that appear syntactically correct - perhaps there's a test that is looking for specific output strings?

Is there a way to run tests locally, so I can update them if needed?

@morgante
Copy link
Contributor

It's actually the linting tests that are failing.

You can run the tests locally using Docker, as documented here.

@morgante
Copy link
Contributor

@ideasculptor FYI it looks like tests are still failing.

@ideasculptor
Copy link
Author

Yep, I’ve been busy with other stuff this morning. I’ll get the tests running locally and get it fixed up. I’ve got other PR’s that need completing, too.

@ideasculptor
Copy link
Author

It looks like all of the current PR's are now failing, which makes me suspect a problem that is independent of the PRs. I've updated the code to include updated docs and to ensure it passes the lint test locally, but it is still failing here. Or, at least, something is failing, but I don't have visibility into what.

I tried running the integration tests locally, but that is failing with a permissions problem that doesn't make sense to me, since I do have ProjectCreator role in the folder it creates, so I'm guessing that is the breakage referenced above. As such, it doesn't seem valuable to try to keep digging into it locally, since I don't think the failure is attributable to my changes.

@ideasculptor
Copy link
Author

Incidentally, is there a command I can use to perform state cleanup in the tests? It is now trying to create a project that it already created and then deleted when there was a failure. I need to taint the resource that generates the suffix on the project id, but I can't figure out an easy way to do so without disassembling all of the scripts and manually performing all the steps needed to be able run terraform taint.

@morgante
Copy link
Contributor

morgante commented Oct 25, 2019

@ideasculptor You can run docker_test_cleanup: https://github.com/terraform-google-modules/terraform-google-project-factory/blob/master/Makefile#L49

I'll try to dig into why things are failing shortly.

@ideasculptor
Copy link
Author

ideasculptor commented Oct 25, 2019

It is dying with the following output. The log files mentioned don't contain any interesting content.

       module.project-factory.module.project-factory.google_project.main: Still creating... [1m40s elapsed]
       
       Error: Error deleting default network in project pf-ci-test-minimal-ffc7ac-9c66: Error waiting for Deleting Network: The network resource 'projects/pf-ci-test-minimal-ffc7ac-9c66/global/networks/default' is already being used by 'projects/pf-ci-test-minimal-ffc7ac-9c66/global/firewalls/default-allow-ssh'
       
       
         on ../../../modules/core_project_factory/main.tf line 126, in resource "google_project" "main":
        126: resource "google_project" "main" {
       
       
>>>>>> ------Exception-------
>>>>>> Class: Kitchen::ActionFailed
>>>>>> Message: 1 actions failed.
>>>>>>     Converge failed on instance <minimal-local>.  Please see .kitchen/logs/minimal-local.log for more details
>>>>>> ----------------------
>>>>>> Please see .kitchen/logs/kitchen.log for more details
>>>>>> Also try running `kitchen diagnose --all` for configuration

That kind of feels like an error in the underlying gcloud api or the terraform provider, since it seems to be objecting to creating a google_project resource with auto_create_network = false

@morgante
Copy link
Contributor

@ideasculptor
Copy link
Author

Forcing it to 2.18.1 does cause the tests to pass locally, but short of committing my changes which set the version explicitly to ~> 2.18.1, I'm not sure how to get the CI pipeline here to grab the latest version of the provider. I'd have thought it would do that automatically, by virtue of its availability, but I guess it just uses the latest version that it has in cache which satisfies the ~> 2.1 requirement that was the original value.

@ideasculptor
Copy link
Author

this was approved but not merged. Any chance it can be merged? I'd love to get away from my fork and back on the mainline.

@morgante morgante merged commit 4870a65 into terraform-google-modules:master Oct 29, 2019
@morgante
Copy link
Contributor

@ideasculptor Thanks, I was just waiting for CI to verify before merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

group name should pass to outputs in order to daisy chain module calls
2 participants