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

feat: ocp-quick-start-DA #591

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

feat: ocp-quick-start-DA #591

wants to merge 11 commits into from

Conversation

Ak-sky
Copy link
Member

@Ak-sky Ak-sky commented Feb 6, 2025

Description

OCP Quickstart variation.

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.

@Ak-sky
Copy link
Member Author

Ak-sky commented Feb 6, 2025

/run pipeline

@Ak-sky
Copy link
Member Author

Ak-sky commented Feb 6, 2025

/run pipeline

1 similar comment
@Ak-sky
Copy link
Member Author

Ak-sky commented Feb 6, 2025

/run pipeline

@Ak-sky Ak-sky marked this pull request as ready for review February 6, 2025 08:58
@Ak-sky
Copy link
Member Author

Ak-sky commented Feb 6, 2025

/run pipeline

@Ak-sky Ak-sky changed the title feat: ocp-quick-start-DA [WIP]feat: ocp-quick-start-DA Feb 6, 2025
@Ak-sky Ak-sky changed the title [WIP]feat: ocp-quick-start-DA feat: ocp-quick-start-DA Feb 9, 2025
@Ak-sky
Copy link
Member Author

Ak-sky commented Feb 9, 2025

/run pipeline

mark_ready: true
- name: cross_kms_support
variations:
- name: quickstart-vpc
Copy link
Member

Choose a reason for hiding this comment

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

this needs to match the name you gave in the ibm_catalog.json

"required": true
},
{
"key": "use_existing_resource_group"
Copy link
Member

Choose a reason for hiding this comment

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

force this to true so it lives beside the resource_group_name inputs in the UI (see internal issue)

"key": "cluster_name"
},
{
"key": "ocp_version"
Copy link
Member

Choose a reason for hiding this comment

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

add a custom dropdown box with the supported versions

"key": "machine_type"
},
{
"key": "vpc_id"
Copy link
Member

Choose a reason for hiding this comment

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

use the VPC picker drodown

{
"key": "region",
"required": true,
"options": [
Copy link
Member

Choose a reason for hiding this comment

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

you dont need to use a custom dropdown here. You can use the out of the box VPC region picker since we only support VPC clusters


provider "ibm" {
ibmcloud_api_key = var.ibmcloud_api_key
region = var.region
Copy link
Member

Choose a reason for hiding this comment

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

expose provider visability


variable "operating_system" {
type = string
description = "Allowed OS values are RHEL 8 (REDHAT_8_64) or Red Hat Enterprise Linux CoreOS (RHCOS). RHCOS requires VPC clusters created from 4.15 onwards. Upgraded clusters from 4.14 cannot use RHCOS."
Copy link
Member

Choose a reason for hiding this comment

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

RHEL 9 was recently added, please add support for that too

default = "bx2.4x16"
}

variable "operating_system" {
Copy link
Member

Choose a reason for hiding this comment

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

This is missing from the ibm_catalog.json? If you pull in the latest common-dev-assets and run pre-commit hooks, it will tell you all the missing vars from ibm_catalog.json.
You can add a dropdown box with the supported values then

variable "cluster_name" {
type = string
description = "The name of the new IBM Cloud OpenShift Cluster. If a `prefix` input variable is specified, it is added to this name in the `<prefix>-value` format."
default = "base-ocp"
Copy link
Member

Choose a reason for hiding this comment

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

hmm, not sure about adding a default value here - every cluster will end up getting called dev-base-ocp and you will get conflicts. Perhaps we make this required?

variable "existing_cos_id" {
type = string
description = "The COS id of an already existing COS instance to use for OpenShift internal registry storage."
default = null
Copy link
Member

@ocofaigh ocofaigh Feb 10, 2025

Choose a reason for hiding this comment

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

This should be required input, with no default value. We don't want the DA to create COS. It should define COS as a dependency in the ibm_catalog.json so we make use of the addons UI

#############################################################################

module "slz_vpc" {
source = "terraform-ibm-modules/landing-zone-vpc/ibm"
Copy link
Member

Choose a reason for hiding this comment

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

I wouldn't use SLZ for the quickstart pattern since quickstart is not going to be locked down

@@ -156,3 +162,76 @@ func TestFSCloudInSchematic(t *testing.T) {
err := options.RunSchematicTest()
assert.Nil(t, err, "This should not have errored")
}

Copy link
Member

Choose a reason for hiding this comment

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

We need an upgrade test for every DA variation too. You should be able to just replace the current upgrade test using the example with a DA upgrade test - we don't need both

"name": "quickstart",
"install_type": "fullstack",
"working_directory": "solutions/quickstart-vpc",
"configuration": [
Copy link
Member

Choose a reason for hiding this comment

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

you need to define dependencies (COS and VPC) and wire the inputs to make it work with the addons UI

Copy link
Member

@ocofaigh ocofaigh left a comment

Choose a reason for hiding this comment

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

Left an initial few comments

@@ -0,0 +1,12 @@
terraform {
required_version = ">=1.3.0"
Copy link
Member

Choose a reason for hiding this comment

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

The module was just updated to require >=1.9.0 so please also update here. It means you can use cross variable validation now too if you find an applicable use case in the DA inputs

required_version = ">=1.3.0"

# Ensure that there is always 1 example locked into the lowest provider version of the range defined in the main
# module's version.tf (basic and add_rules_to_sg), and 1 example that will always use the latest provider version (advanced, fscloud and multiple mzr).
Copy link
Member

Choose a reason for hiding this comment

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

Remove the above comments, they are valid for an example, not a DA. For the DA just leave this comment:

# Lock DA into an exact provider version - renovate automation will keep it updated

required_providers {
ibm = {
source = "IBM-Cloud/ibm"
version = "1.70.0"
Copy link
Member

Choose a reason for hiding this comment

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

bump to latest

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.

2 participants