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

test: refactor basic test #567

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

test: refactor basic test #567

wants to merge 1 commit into from

Conversation

shemau
Copy link
Contributor

@shemau shemau commented Jan 14, 2025

Description

Discussion example of how tests may be refactored, based on basic test.

Release required?

  • No release
  • Patch release (x.x.X)
  • Minor release (x.X.x)
  • Major release (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:

/run pipeline

Checklist for reviewers

  • If relevant, a test for the change is included or updated with this PR.
  • If relevant, documentation for the change is included or updated with this PR.

For mergers

  • Use a conventional commit message to set the release level. Follow the guidelines.
  • Include information that users need to know about the PR in the commit message. The commit message becomes part of the GitHub release notes.
  • Use the Squash and merge option.

@shemau shemau marked this pull request as draft January 14, 2025 12:41
Copy link
Contributor Author

@shemau shemau left a comment

Choose a reason for hiding this comment

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

These files should be mostly just copied, cookie cutter style to other ICD types. Minimal references in the examples folder, with the differences be covered in the tests folder.

Comment on lines 6 to +7
- Create a new ICD Postgresql database instance.
- Create a read-only replica of the leader Postgresql database instance. For more info on Read-only Replicas, see [Configuring Read-only Replicas](https://cloud.ibm.com/docs/databases-for-postgresql?topic=databases-for-postgresql-read-only-replicas)
- Create a read-only replica of the leader database instance. For more info on Read-only Replicas, see [Configuring Read-only Replicas](https://cloud.ibm.com/docs/databases-for-postgresql?topic=databases-for-postgresql-read-only-replicas)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

A few references to the ICD type (postgres) still remain in the readme for clarity

name = "${var.prefix}-postgres"
pg_version = var.pg_version
name = "${var.prefix}-data-store"
pg_version = var.db_version
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This becomes the only line in this file that needs to change from one ICD type to another.

name = "${var.prefix}-postgres"
pg_version = var.pg_version
name = "${var.prefix}-data-store"
pg_version = var.db_version
region = var.region
resource_tags = var.resource_tags
access_tags = var.access_tags
member_host_flavor = var.member_host_flavor
Copy link
Contributor Author

Choose a reason for hiding this comment

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

To support EDB, which does not support multitenant. Default is multitenant.

Comment on lines +5 to +6
description = "Database instance id"
value = module.database.id
Copy link
Contributor Author

Choose a reason for hiding this comment

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

All outputs are generic. This file can be used unchanged in every ICD type

Comment on lines +23 to +34
module "replica" {
source = "../.."
resource_group_id = module.resource_group.resource_group_id
name = "${var.prefix}-replica"
pg_version = var.db_version
region = var.region
resource_tags = var.resource_tags
access_tags = var.access_tags
member_host_flavor = var.member_host_flavor
remote_leader_crn = module.database.crn
depends_on = [time_sleep.wait_time]
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved to a new file. This file can be include in the few ICD types that support replicas and excluded from the others.

Comment on lines -41 to -52
variable "read_only_replicas_count" {
type = number
description = "Number of read-only replicas per leader"
default = 1
validation {
condition = alltrue([
var.read_only_replicas_count >= 1,
var.read_only_replicas_count <= 5
])
error_message = "There is a limit of five read-only replicas per leader"
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unnecessary complication for an example. Hard code it to 1.

variable "member_host_flavor" {
type = string
description = "Allocated host flavor per member. For more information, see https://registry.terraform.io/providers/IBM-Cloud/ibm/latest/docs/resources/database#host_flavor"
default = null
default = "multitenant"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Use multitenant for default, will need to override for EDB where it is not supported.

This file can be copied to all ICD types with no changes, but EDB should change the default.

Comment on lines +54 to +55
"db_version": "13",
"member_host_flavor": "multitenant",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only difference to this test between ICD types is the vars.
Should pinned to minimum supported ICD version.
host flavor could probably be removed here... since it will default to a supported value.

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.

1 participant