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

Infrastructure for creating a binary authentication attestor #530

Merged
merged 8 commits into from May 27, 2020
Merged

Infrastructure for creating a binary authentication attestor #530

merged 8 commits into from May 27, 2020

Conversation

mike-ensor
Copy link
Contributor

This is a module that can be used to create an Attestor for the Binary Authorization workflow. Outputs include the full path/name to the attestor and the full path/name to the associated key

@mike-ensor mike-ensor requested review from bharathkkb, Jberlinsky and a team as code owners May 21, 2020 19:55
Copy link
Member

@bharathkkb bharathkkb 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 the PR!

  • Looks like we are missing a Readme similar to this

  • Additionally it would be great, if you can add an example with this module

Copy link
Member

@bharathkkb bharathkkb left a comment

Choose a reason for hiding this comment

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

Looks like the CI failed to run on latest commit due to a GitHub outage.

Left a few comments, particularly some nits the linter may have caught if it ran. You can also run it locally using the command make docker_test_lint. More details here.

Additionally most of our modules have an example, which we use to showcase how the GKE module works alongside other add-on modules. As this can exist without a referencing a GKE cluster, I will defer to @morgante on how to proceed.

modules/binary-authorization/outputs.tf Outdated Show resolved Hide resolved
modules/binary-authorization/variables.tf Outdated Show resolved Hide resolved
modules/binary-authorization/README.md Outdated Show resolved Hide resolved
… of files and automated hooks for auto-documenting feature
Copy link
Member

@bharathkkb bharathkkb left a comment

Choose a reason for hiding this comment

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

LGTM overall.
Tests failed due to missing variable project_id.

modules/binary-authorization/main.tf Show resolved Hide resolved
modules/binary-authorization/README.md Show resolved Hide resolved
Co-authored-by: Bharath KKB <bharathkrishnakb@gmail.com>
Copy link
Member

@bharathkkb bharathkkb left a comment

Choose a reason for hiding this comment

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

@mike-ensor looks like just these two needs to be fixed for linting to pass #530 (comment) and #530 (comment). You can also run it locally using the command make docker_test_lint. More details here.

@mike-ensor
Copy link
Contributor Author

@mike-ensor looks like just these two needs to be fixed for linting to pass #530 (comment) ...

I've updated the outputs file, run both the "generate docs" and "lint" tools without errors.

@mike-ensor
Copy link
Contributor Author

@bharathkkb I'm not sure why the build is failing in GitHub, but it might be related to the failure of the ./test/setup folder not validating fully.

@bharathkkb
Copy link
Member

@mike-ensor looks like make docker_generate_docs was not re run after you added project_id variable. It's complaining the documentation is out of date. If you rerun that and commit changes, it should be good.

@mike-ensor
Copy link
Contributor Author

@bharathkkb yes, I didn't run the create docs after :-(

the build is running now

@morgante morgante merged commit cc30fbb into terraform-google-modules:master May 27, 2020
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.

None yet

3 participants