-
Notifications
You must be signed in to change notification settings - Fork 157
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
[TF-9605] Add Private Registry Module Testing #1096
Conversation
e31531e
to
8ab253d
Compare
8a3824c
to
d9da171
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No smoke test yet. Just some initial quick observations and questions so far.
CHANGELOG.md
Outdated
## UNRELEASED | ||
|
||
Features | ||
* `d/tfe_registry_module`: Add `test_config` and `vcs_repo.tags` attributes, by @hashimoon [1096](https://github.com/hashicorp/terraform-provider-tfe/pull/1096) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, publishing_mechanism
and vcs_repo.branch
-- It might be worth quickly explaining that these are related in the sense that they allow module developers more release flexibility but I don't have the words
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Smoke test feedback:
- creating RM with tags set to
true
creates RM with tag => false, config used:
resource "tfe_registry_module" "test-with-tags-only-true" {
vcs_repo {
display_identifier = ""
identifier = ""
oauth_token_id = "id"
tags = true
}
}
Should be able to create tags with true value
- creating RM with test_config.tests_enabled set to
true
creates RM with tests_enabled => false, config used:
resource "tfe_registry_module" "test-with-test-config-set-to-true" {
test_config {
tests_enabled = true
}
vcs_repo {
display_identifier = ""
identifier = ""
oauth_token_id = "id"
tags = true
}
}
Should be able to create tests_enabled
with true value
- creating a RM without adding the test_config block is successful. Attempting a new apply without any config changes, tries to unsets the test_config block:
# tfe_registry_module.test-with-branch-only will be updated in-place
~ resource "tfe_registry_module" "test-with-branch-only" {
id = "mod-1"
name = "tfe-module"
# (6 unchanged attributes hidden)
- test_config {
- tests_enabled = false -> null
}
# (1 unchanged block hidden)
}
13f2812
to
8c76c27
Compare
Do you have any more context on this? Can you see share what value is return for the
This is expected behavior. Tests are only supported for modules with branch-based publishing.
This is expected behavior. Tests are only supported for Registry Modules with branch-based publishing at the moment. |
e7f9532
to
00b2886
Compare
00b2886
to
9f6fc4a
Compare
9f6fc4a
to
0005396
Compare
0005396
to
8417e8f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good progress, see our slack chat for other comments with usecase examples.
@@ -125,6 +155,9 @@ The following arguments are supported: | |||
* `namespace` - (Optional) The namespace of a public registry module. It can be used if `module_provider` is set and `registry_name` is public. | |||
* `registry_name` - (Optional) Whether the registry module is private or public. It can be used if `module_provider` is set. | |||
|
|||
The `test_config` block supports | |||
* `tests_enabled` - (Required) Specifies whether tests run for the registry module. Tests are only supported for branch-based publishing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the implementation sets both test_config
and tests_enabled
as optional attributes, but here it is state as required...update to reflect same requirement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
1ce75a4
to
1585890
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Smoke testing feedback:
Most of the config is working as expected 🎉
Found 2 config with issues below
1.
Actual:
successful apply
..s of the config below always ends up in a state with changes to update. A second apply seems to get stuck in an infinite state trying to modify. I ctrl c
to stop the process.
Expected:
after an apply the resource state should reflect created resource
Config:
resource "tfe_registry_module" "test-with-branch-tags-and-test-config" {
test_config {
tests_enabled = true
}
vcs_repo {
display_identifier = "Uk1288/terraform-google-cloud-dns"
identifier = "Uk1288/terraform-google-cloud-dns"
oauth_token_id = "ot-ID..."
branch = "main"
tags = true
}
}
Results in changes to update:
~ vcs_repo {
~ tags = false -> true
# (4 unchanged attributes hidden)
}
Actual:
provider crashes.
Expected:
should successfully apply or return an error
Config:
resource "tfe_registry_module" "test-with-branch-tags-and-test-config" {
test_config {
}
vcs_repo {
display_identifier = "Uk1288/terraform-google-cloud-dns"
identifier = "Uk1288/terraform-google-cloud-dns"
oauth_token_id = "ot-ID..."
branch = "main"
}
}
bef01d0
to
7a1e4d5
Compare
7a1e4d5
to
73f87a6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gave this another go, working as expected. Approving because functionality is working as expected.
🍹 One of my configs seems to be reset even after a successful apply. Expected behaviour is for state changes to reflect the resources after a successful apply.
Config:
resource "tfe_registry_module" "test-without-test-config-block" {
vcs_repo {
display_identifier = "Uk1288/terraform-aws-simple-s3-cdn"
identifier = "Uk1288/terraform-aws-simple-s3-cdn"
oauth_token_id = "ot-id..."
branch = "main"
tags = false
}
}
After a successful apply still results in
# tfe_registry_module.test-without-test-config-block will be updated in-place
~ resource "tfe_registry_module" "test-without-test-config-block" {
id = "mod-..."
name = "simple-s3-cdn"
# (6 unchanged attributes hidden)
- test_config {
- tests_enabled = false -> null
}
# (1 unchanged block hidden)
}
Plan: 0 to add, 1 to change, 0 to destroy.
Expected behaviour to result in no changes to apply.
Description
Add the following attributes to
RegistryModule
to support testingpublishing_mechanism
vcs_repo.branch
vcs_repo.tags
test_config.tests_enabled
Remember to:
Testing plan
RegistryModule
publishing_mechanism
isgit_tag
vcs_repo.branch
to a branch on the git repositoryvcs_repo.tags
tofalse
test_config.tests_enabled
totrue
publishing_mechanism
isbranch
External links
Include any links here that might be helpful for people reviewing your PR. If there are none, feel free to delete this section.
Output from acceptance tests
Please run applicable acceptance tests locally and include the output here. See testing.md to learn how to run acceptance tests.
If you are an external contributor, your contribution(s) will first be reviewed before running them against the project's CI pipeline.