feat: add new code engine module #5
Conversation
|
/run pipeline |
2 similar comments
|
/run pipeline |
|
/run pipeline |
|
/run pipeline |
1 similar comment
|
/run pipeline |
.github/settings.yml
Outdated
| description: "Creates IBM Cloud Code Engine resources." | ||
|
|
||
| # Use a comma-separated list of topics to set on the repo (ensure not to use any caps in the topic string). | ||
| topics: terraform, ibm-cloud, terraform-module |
There was a problem hiding this comment.
please add these topics too: core-team, code-engine, stable, supported
|
/run pipeline |
README.md
Outdated
|
|
||
|
|
||
| <!-- This heading should always match the name of the root level module (aka the repo name) --> | ||
| ## terraform-ibm-module-template |
There was a problem hiding this comment.
terraform-ibm-code-engine
README.md
Outdated
| Use real values instead of "var.<var_name>" or other placeholder values | ||
| unless real values don't help users know what to change. | ||
| --> | ||
| source = "../.." |
There was a problem hiding this comment.
this isnt properly formatted:
README.md
Outdated
| --> | ||
| source = "../.." | ||
| resource_group_id = module.resource_group.resource_group_id | ||
| project_name = "${var.prefix}-project" |
There was a problem hiding this comment.
can you use real world like values here instead of var.
shemau
left a comment
shemau
left a comment
There was a problem hiding this comment.
There is a test coverage issue, in that neither binding or domain_mapping are tested anywhere.
These a potentially more complex tests, in that a binding requires a service to bind to, along with a process to generate a service access key. I am not sure about a domain mapping (knowledge gap).
modules/binding/variables.tf
Outdated
| name = string | ||
| resource_type = string | ||
| })) | ||
| default = [] |
There was a problem hiding this comment.
This is a required property, so this default does not work. I think it should be required.
modules/binding/main.tf
Outdated
| secret_name = var.secret_name | ||
|
|
||
| dynamic "component" { | ||
| for_each = var.components != null ? var.components : [] |
There was a problem hiding this comment.
Also noted in the variables.tf comment. component is a required property, so var.components should never be null and the for_each should never run off the empty list.
modules/domain_mapping/variables.tf
Outdated
| name = string | ||
| resource_type = string | ||
| })) | ||
| default = [] |
There was a problem hiding this comment.
This is a required property, no default required.
modules/domain_mapping/main.tf
Outdated
| name = var.name | ||
| tls_secret = var.tls_secret | ||
| dynamic "component" { | ||
| for_each = var.components != null ? var.components : [] |
There was a problem hiding this comment.
This is required, so var.components should not be null and passing an empty list will cause an issue at apply.
|
/run pipeline |
|
/run pipeline |
|
/run pipeline |
|
/run pipeline |
|
/run pipeline |
|
/run pipeline |
|
/run pipeline |
|
/run pipeline |
|
/run pipeline |
|
I haven't reviewed the code (I'll delegate that to @shemau ) however I think it would be better if you had examples named |
|
/run pipeline |
|
/run pipeline |
|
🎉 This PR is included in version 1.0.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Description
Release required?
x.x.X)x.X.x)X.x.x)Release notes content
Run the pipeline
If the CI pipeline doesn't run when you create the PR, the PR requires a user with GitHub collaborators access to run the pipeline.
Run the CI pipeline when the PR is ready for review and you expect tests to pass. Add a comment to the PR with the following text:
Checklist for reviewers
For mergers