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.

Sign up for GitHub

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

Jump to bottom

{Core} Move clouds.config to config dir #26138

Draft
wants to merge 4 commits into
base: dev
Choose a base branch
from
Draft

{Core} Move clouds.config to config dir #26138

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
71f95ed
Init
bebound Apr 13, 2023
acd8f2c
Minor fix
bebound Apr 13, 2023
031d92b
Fix test
bebound Apr 14, 2023
92a084f
Minor fix
bebound Apr 17, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 25 additions & 19 deletions src/azure-cli-core/azure/cli/core/cloud.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,14 @@

logger = get_logger(__name__)

# This default cloud name. If you use DummyCli(random_config_dir=True), use get_cloud_config_file() instead.
CLOUD_CONFIG_FILE = os.path.join(GLOBAL_CONFIG_DIR, 'clouds.config')
Copy link
Member

@jiasli jiasli Apr 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

The original implementation is wrong.

get_default_cli uses GLOBAL_CONFIG_DIR for knack.cli.CLI.__init__'s config_dir argument:

azure-cli/src/azure-cli-core/azure/cli/core/__init__.py

Lines 902 to 903 in 63121ed

return AzCli(cli_name='az',
config_dir=GLOBAL_CONFIG_DIR,

The location of clouds.config is also built from GLOBAL_CONFIG_DIR.

If knack.cli.CLI.__init__'s config_dir doesn't use GLOBAL_CONFIG_DIR, but another location, like what has been done in #25689:

azure-cli/src/azure-cli-core/azure/cli/core/mock.py

Lines 28 to 29 in b223dd9

config_dir=os.path.join(GLOBAL_CONFIG_DIR, 'dummy_cli_config_dir',
random_string()) if random_config_dir else GLOBAL_CONFIG_DIR,

Then the location of clouds.config will still be under GLOBAL_CONFIG_DIR, because it directly uses CLOUD_CONFIG_FILE, instead of cli_ctx.config.config_dir. This defeats the purpose of overriding config_dir.

Copy link
Member

Choose a reason for hiding this comment

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

profile._storage is an azure.cli.core._session.ACCOUNT which is a persisted JSON. It can easily be replaced by an in-memory dict during tests, but cloud.config is an INI file, which introduces inconsistency. See #14436 (comment)



def get_cloud_config_file(cli_ctx=None):
return os.path.join(cli_ctx.config.config_dir, 'clouds.config') if cli_ctx else CLOUD_CONFIG_FILE
Comment on lines +25 to +26
Copy link
Member

Choose a reason for hiding this comment

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

Calling this function multiple times is tedious. Perhaps we simply save it as cli_ctx.cloud_config?



# Add names of clouds that don't allow telemetry data collection here such as some air-gapped clouds.
CLOUDS_FORBIDDING_TELEMETRY = ['USSec', 'USNat']

Expand Down Expand Up @@ -511,10 +517,10 @@ def get_clouds(cli_ctx):
for c in KNOWN_CLOUDS:
_config_add_cloud(config, c)
try:
config.read(CLOUD_CONFIG_FILE)
config.read(get_cloud_config_file(cli_ctx))
except configparser.MissingSectionHeaderError:
os.remove(CLOUD_CONFIG_FILE)
logger.warning("'%s' is in bad format and has been removed.", CLOUD_CONFIG_FILE)
os.remove(get_cloud_config_file(cli_ctx))
logger.warning("'%s' is in bad format and has been removed.", get_cloud_config_file(cli_ctx))
for section in config.sections():
c = Cloud(section)
for option in config.options(section):
Expand Down Expand Up @@ -563,9 +569,9 @@ def get_active_cloud(cli_ctx=None):
return get_cloud(cli_ctx, default_cloud_name)


def get_cloud_subscription(cloud_name):
def get_cloud_subscription(cloud_name, cli_ctx=None):
config = configparser.ConfigParser()
config.read(CLOUD_CONFIG_FILE)
config.read(get_cloud_config_file(cli_ctx))
try:
return config.get(cloud_name, 'subscription')
except (configparser.NoOptionError, configparser.NoSectionError):
Expand All @@ -576,7 +582,7 @@ def set_cloud_subscription(cli_ctx, cloud_name, subscription):
if not _get_cloud(cli_ctx, cloud_name):
raise CloudNotRegisteredException(cloud_name)
config = configparser.ConfigParser()
config.read(CLOUD_CONFIG_FILE)
config.read(get_cloud_config_file(cli_ctx))
if subscription:
try:
config.add_section(cloud_name)
Expand All @@ -588,17 +594,17 @@ def set_cloud_subscription(cli_ctx, cloud_name, subscription):
config.remove_option(cloud_name, 'subscription')
except configparser.NoSectionError:
pass
if not os.path.isdir(GLOBAL_CONFIG_DIR):
os.makedirs(GLOBAL_CONFIG_DIR)
with open(CLOUD_CONFIG_FILE, 'w') as configfile:
if not os.path.isdir(os.path.dirname(get_cloud_config_file(cli_ctx))):
os.makedirs(os.path.dirname(get_cloud_config_file(cli_ctx)))
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 folder has been created by

azure-cli/src/azure-cli-core/azure/cli/core/__init__.py

Lines 77 to 78 in 63121ed

azure_folder = self.config.config_dir
ensure_dir(azure_folder)

I don't know if this function is called when cli_ctx does not exist.

with open(get_cloud_config_file(cli_ctx), 'w') as configfile:
config.write(configfile)


def _set_active_subscription(cli_ctx, cloud_name):
from azure.cli.core._profile import (Profile, _ENVIRONMENT_NAME, _SUBSCRIPTION_ID,
_STATE, _SUBSCRIPTION_NAME)
profile = Profile(cli_ctx=cli_ctx)
subscription_to_use = get_cloud_subscription(cloud_name) or \
subscription_to_use = get_cloud_subscription(cloud_name, cli_ctx) or \
next((s[_SUBSCRIPTION_ID] for s in profile.load_cached_subscriptions() # noqa
if s[_STATE] == 'Enabled'),
None)
Expand Down Expand Up @@ -644,26 +650,26 @@ def _config_add_cloud(config, cloud, overwrite=False):
config.set(cloud.name, 'suffix_{}'.format(k), v)


def _save_cloud(cloud, overwrite=False):
def _save_cloud(cloud, overwrite=False, cli_ctx=None):
config = configparser.ConfigParser()
config.read(CLOUD_CONFIG_FILE)
config.read(get_cloud_config_file(cli_ctx))
_config_add_cloud(config, cloud, overwrite=overwrite)
if not os.path.isdir(GLOBAL_CONFIG_DIR):
os.makedirs(GLOBAL_CONFIG_DIR)
with open(CLOUD_CONFIG_FILE, 'w') as configfile:
if not os.path.isdir(os.path.dirname(get_cloud_config_file(cli_ctx))):
os.makedirs(os.path.dirname(get_cloud_config_file(cli_ctx)))
with open(get_cloud_config_file(cli_ctx), 'w') as configfile:
config.write(configfile)


def add_cloud(cli_ctx, cloud):
if _get_cloud(cli_ctx, cloud.name):
raise CloudAlreadyRegisteredException(cloud.name)
_save_cloud(cloud)
_save_cloud(cloud, cli_ctx=cli_ctx)


def update_cloud(cli_ctx, cloud):
if not _get_cloud(cli_ctx, cloud.name):
raise CloudNotRegisteredException(cloud.name)
_save_cloud(cloud, overwrite=True)
_save_cloud(cloud, overwrite=True, cli_ctx=cli_ctx)


def remove_cloud(cli_ctx, cloud_name):
Expand All @@ -677,9 +683,9 @@ def remove_cloud(cli_ctx, cloud_name):
raise CannotUnregisterCloudException("The cloud '{}' cannot be unregistered "
"as it's not a custom cloud.".format(cloud_name))
config = configparser.ConfigParser()
config.read(CLOUD_CONFIG_FILE)
config.read(get_cloud_config_file(cli_ctx))
config.remove_section(cloud_name)
with open(CLOUD_CONFIG_FILE, 'w') as configfile:
with open(get_cloud_config_file(cli_ctx), 'w') as configfile:
config.write(configfile)


Expand Down
Loading