Skip to content

Conversation

@iamar7
Copy link
Member

@iamar7 iamar7 commented Nov 11, 2025

Description

Resolves: #102

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.

Copy link
Contributor

@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.

Have you run the script in an account where no primary metadata region has been set? What happens?

The script is currently hard coded to production endpoints, meaning it wont work in stage. The script needs to be able to handle environment variable overrides that someone might make. Same way we did in confirm_lb_active.sh for example.

@iamar7
Copy link
Member Author

iamar7 commented Nov 12, 2025

Have you run the script in an account where no primary metadata region has been set? What happens?

The script is currently hard coded to production endpoints, meaning it wont work in stage. The script needs to be able to handle environment variable overrides that someone might make. Same way we did in confirm_lb_active.sh for example.

In an account where the primary_metadata_region is not set the script returns an empty string.

Screenshot 2025-11-12 at 10 25 12 PM

@iamar7 iamar7 marked this pull request as ready for review November 17, 2025 09:01
@iamar7 iamar7 requested a review from Khuzaima05 as a code owner November 17, 2025 09:01
@iamar7
Copy link
Member Author

iamar7 commented Nov 17, 2025

/run pipeline

@iamar7
Copy link
Member Author

iamar7 commented Nov 17, 2025

/run pipeline

@iamar7
Copy link
Member Author

iamar7 commented Nov 17, 2025

/run pipeline

@iamar7
Copy link
Member Author

iamar7 commented Nov 18, 2025

This is an expected failure since we were updating the metrics_routing_settings by default. Skipping Upgrade Test

2025/11/17 23:52:14 Terraform plan | Terraform will perform the following actions:
         2025/11/17 23:52:14 Terraform plan | 
         2025/11/17 23:52:14 Terraform plan |   # module.metrics_routing[0].ibm_metrics_router_settings.metrics_router_settings[0] will be destroyed
         2025/11/17 23:52:14 Terraform plan |   # (because index [0] is out of range for count)
         2025/11/17 23:52:14 Terraform plan |   - resource "ibm_metrics_router_settings" "metrics_router_settings" {
         2025/11/17 23:52:14 Terraform plan |       - id                        = "br-sao" -> null
         2025/11/17 23:52:14 Terraform plan |       - permitted_target_regions  = [] -> null
         2025/11/17 23:52:14 Terraform plan |       - primary_metadata_region   = "br-sao" -> null
         2025/11/17 23:52:14 Terraform plan |       - private_api_endpoint_only = false -> null
         2025/11/17 23:52:14 Terraform plan |         # (1 unchanged attribute hidden)
         2025/11/17 23:52:14 Terraform plan |     }
         2025/11/17 23:52:14 Terraform plan | 
Screenshot 2025-11-18 at 6 22 52 AM

@iamar7
Copy link
Member Author

iamar7 commented Nov 18, 2025

/run pipeline

@iamar7
Copy link
Member Author

iamar7 commented Dec 1, 2025

/run pipeline

import sys
import time

import requests
Copy link
Contributor

Choose a reason for hiding this comment

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

We cannot use requests - this is an external pip package that requires installation. I did mention this on the deep dive. We can use the python built in http library: https://docs.python.org/3/library/http.client.html

Check out our internal sdnlb module which recently was updated to use this library too.

Copy link
Contributor

@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.

@iamar7
Copy link
Member Author

iamar7 commented Dec 5, 2025

/run pipeline

@iamar7
Copy link
Member Author

iamar7 commented Dec 5, 2025

/run pipeline

@iamar7 iamar7 requested a review from ocofaigh December 5, 2025 15:16
Copy link
Contributor

@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 some comments

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

variable "region" {
description = "The IBM Cloud Metrics Routing region."
Copy link
Contributor

Choose a reason for hiding this comment

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

A region input variable doesn't really make sense for this module? The goal of the module is to find out if the account has a primary metadata region already set.
So var.region makes no sense here. I can see you are using it to construct the api. But does it matter what region endpoint is even used here? Will they all return the same result? Or even better, is there a global api?

Copy link
Member Author

Choose a reason for hiding this comment

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

It doesn't matter if the region is us-east or us-south since they all will return the same result and I was not able to find a global API. So, do I hardcode the region here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Based on the discussion over call hard-coding the region to us-south

@iamar7
Copy link
Member Author

iamar7 commented Dec 8, 2025

/run pipeline

@iamar7
Copy link
Member Author

iamar7 commented Dec 8, 2025

/run pipeline

@ocofaigh
Copy link
Contributor

@iamar7 Didn't the upgrade test catch a valid breaking change here? If a user upgrades from previous version to the code in this PR, isnt their primary metadata region going to get removed for a period of time and then added back?
Did you do an upgrade test to verify?

@iamar7
Copy link
Member Author

iamar7 commented Dec 12, 2025

@iamar7 Didn't the upgrade test catch a valid breaking change here? If a user upgrades from previous version to the code in this PR, isnt their primary metadata region going to get removed for a period of time and then added back? Did you do an upgrade test to verify?

Screenshot 2025-12-12 at 5 43 03 AM

when I did terraform apply --auto-approve it showed me that the resource is destroyed but in the cloud UI the primary_metadata_region was still showing as us-east which after a minute reverted back to us-south which was the metadata_region before I updated it to us-east when I applied the main branch

@iamar7
Copy link
Member Author

iamar7 commented Dec 12, 2025

I used a fresh account to test it first applied the main branch which updated the primary_metadata_region from null to us-east then switched to this branch where the terraform plan was showing this

  # module.metrics_routing[0].ibm_metrics_router_settings.metrics_router_settings[0] will be destroyed
  # (because index [0] is out of range for count)
  - resource "ibm_metrics_router_settings" "metrics_router_settings" {
      - id                        = "us-east" -> null
      - permitted_target_regions  = [] -> null
      - primary_metadata_region   = "us-east" -> null
      - private_api_endpoint_only = false -> null
        # (1 unchanged attribute hidden)
    }

Plan: 0 to add, 0 to change, 1 to destroy.

I did a apply post that when I checked the primary_metadata_region it was still showing as us-east

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.

Investigate requested change around the automatic setting of the metrics primary metadata region

3 participants