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

fix config validator for region in network client #956

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

snwzd
Copy link

@snwzd snwzd commented Jan 15, 2025

Added region argument while creating network client for terraform validations

How to categorize this PR?

/area robustness
/kind bug
/platform openstack

What this PR does / why we need it:

Which issue(s) this PR fixes:
Fixes #955

Special notes for your reviewer:
While creating networking client for terraform validations, region is not being used as parameter for the function.

Added region argument while creating network client for terraform validations
@snwzd snwzd requested review from a team as code owners January 15, 2025 08:53
@gardener-robot gardener-robot added the needs/review Needs review label Jan 15, 2025
@CLAassistant
Copy link

CLAassistant commented Jan 15, 2025

CLA assistant check
All committers have signed the CLA.

@gardener-robot gardener-robot added area/robustness Robustness, reliability, resilience related kind/bug Bug platform/openstack OpenStack platform/infrastructure labels Jan 15, 2025
@gardener-robot
Copy link

@snwzd Thank you for your contribution.

@gardener-robot gardener-robot added the size/xs Size of pull request is tiny (see gardener-robot robot/bots/size.py) label Jan 15, 2025
@gardener-robot-ci-2
Copy link
Contributor

Thank you @snwzd for your contribution. Before I can start building your PR, a member of the organization must set the required label(s) {'reviewed/ok-to-test'}. Once started, you can check the build status in the PR checks section below.

@snwzd
Copy link
Author

snwzd commented Jan 15, 2025

Hi, This pull request is regarding the fix for the issue issue that I am facing, please review it and tell me if there are any other changes that need to be made.

Copy link
Contributor

@kon-angelo kon-angelo left a comment

Choose a reason for hiding this comment

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

@snwzd thank you for the contribution. Only a minor nit below

Comment on lines 65 to 68
networkingClient, err := clientFactory.Networking(func(opts gophercloud.EndpointOpts) gophercloud.EndpointOpts {
opts.Region = infra.Spec.Region
return opts
})
Copy link
Contributor

Choose a reason for hiding this comment

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

We already should have a function to do just that, so can you give it a try with:

	networkingClient, err := clientFactory.Networking(openstackclient.WithRegion(infra.Spec.Region))

Copy link
Author

Choose a reason for hiding this comment

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

Hi, I've updated that section

Copy link
Contributor

Choose a reason for hiding this comment

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

@snwzd you need to run make generate (according to the test results)

Copy link
Author

Choose a reason for hiding this comment

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

I ran make generate, but it didn’t seem to make much changes. I think it removed the unused gophercloud import, which was the only noticeable change. (probably my editor missed it)

Updated region argument while creating network client for terraform validations
kon-angelo
kon-angelo previously approved these changes Jan 15, 2025
Copy link
Contributor

@kon-angelo kon-angelo left a comment

Choose a reason for hiding this comment

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

/reviewed ok-to-test

@gardener-robot gardener-robot added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jan 15, 2025
@gardener-robot-ci-3 gardener-robot-ci-3 added needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Jan 15, 2025
@AndreasBurger AndreasBurger added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jan 15, 2025
@gardener-robot-ci-3 gardener-robot-ci-3 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jan 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/robustness Robustness, reliability, resilience related kind/bug Bug needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) needs/review Needs review platform/openstack OpenStack platform/infrastructure size/xs Size of pull request is tiny (see gardener-robot robot/bots/size.py)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

config validation for network fails in other regions that are not default
7 participants