-
Notifications
You must be signed in to change notification settings - Fork 1
feat: add module to fetch primary_metadata_region #127
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
base: main
Are you sure you want to change the base?
Conversation
ocofaigh
left a comment
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.
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
|
|
/run pipeline |
|
/run pipeline |
|
/run pipeline |
|
/run pipeline |
|
/run pipeline |
|
/run pipeline |
| import sys | ||
| import time | ||
|
|
||
| import requests |
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.
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.
ocofaigh
left a comment
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.
|
/run pipeline |
|
/run pipeline |
ocofaigh
left a comment
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.
left some comments
| ############################################################################## | ||
|
|
||
| variable "region" { | ||
| description = "The IBM Cloud Metrics Routing region." |
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.
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?
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.
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?
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.
Based on the discussion over call hard-coding the region to us-south
|
/run pipeline |
|
/run pipeline |


Description
Resolves: #102
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