Refactor anaconda_token_rotation.rotate_anaconda_token

[ytausch]

Comment:

Where is the rotate-anaconda-token CLI command used?

If it's not used, the corresponding code should be removed.

If it's used, the anaconda_token_rotation.rotate_anaconda_token method should be refactored because it's seriously broken.

conda-smithy/conda_smithy/anaconda_token_rotation.py

Lines 34 to 203 in 9e4720f

	def rotate_anaconda_token(
	user,
	project,
	feedstock_config_path,
	drone=True,
	circle=True,
	travis=True,
	azure=True,
	appveyor=True,
	github_actions=True,
	token_name="BINSTAR_TOKEN",
	drone_endpoints=(),
	):
	"""Rotate the anaconda (binstar) token used by the CI providers
	All exceptions are swallowed and stdout/stderr from this function is
	redirected to `/dev/null`. Sanitized error messages are
	displayed at the end.
	If you need to debug this function, define `DEBUG_ANACONDA_TOKENS` in
	your environment before calling this function.
	"""
	# we are swallong all of the logs below, so we do a test import here
	# to generate the proper errors for missing tokens
	# note that these imports cover all providers
	from .ci_register import travis_endpoint # noqa
	from .azure_ci_utils import default_config # noqa
	from .github import gh_token
	anaconda_token = _get_anaconda_token()
	if github_actions:
	gh = Github(gh_token())
	# capture stdout, stderr and suppress all exceptions so we don't
	# spill tokens
	failed = False
	err_msg = None
	with open(os.devnull, "w") as fp:
	if "DEBUG_ANACONDA_TOKENS" in os.environ:
	fpo = sys.stdout
	fpe = sys.stderr
	else:
	fpo = fp
	fpe = fp
	with redirect_stdout(fpo), redirect_stderr(fpe):
	try:
	if circle:
	try:
	rotate_token_in_circle(
	user, project, anaconda_token, token_name
	)
	except Exception as e:
	if "DEBUG_ANACONDA_TOKENS" in os.environ:
	raise e
	else:
	err_msg = (
	"Failed to rotate token for %s/%s"
	" on circle!"
	) % (user, project)
	failed = True
	raise RuntimeError(err_msg)
	if drone:
	for drone_endpoint in drone_endpoints:
	try:
	rotate_token_in_drone(
	user,
	project,
	anaconda_token,
	token_name,
	drone_endpoint,
	)
	except Exception as e:
	if "DEBUG_ANACONDA_TOKENS" in os.environ:
	raise e
	else:
	err_msg = (
	"Failed to rotate token for %s/%s"
	" on drone endpoint %s!"
	) % (user, project, drone_endpoint)
	failed = True
	raise RuntimeError(err_msg)
	if travis:
	try:
	rotate_token_in_travis(
	user,
	project,
	feedstock_config_path,
	anaconda_token,
	token_name,
	)
	except Exception as e:
	if "DEBUG_ANACONDA_TOKENS" in os.environ:
	raise e
	else:
	err_msg = (
	"Failed to rotate token for %s/%s"
	" on travis!"
	) % (user, project)
	failed = True
	raise RuntimeError(err_msg)
	if azure:
	try:
	rotate_token_in_azure(
	user, project, anaconda_token, token_name
	)
	except Exception as e:
	if "DEBUG_ANACONDA_TOKENS" in os.environ:
	raise e
	else:
	err_msg = (
	"Failed to rotate token for %s/%s" " on azure!"
	) % (user, project)
	failed = True
	raise RuntimeError(err_msg)
	if appveyor:
	try:
	rotate_token_in_appveyor(
	feedstock_config_path, anaconda_token, token_name
	)
	except Exception as e:
	if "DEBUG_ANACONDA_TOKENS" in os.environ:
	raise e
	else:
	err_msg = (
	"Failed to rotate token for %s/%s"
	" on appveyor!"
	) % (user, project)
	failed = True
	raise RuntimeError(err_msg)
	if github_actions:
	try:
	rotate_token_in_github_actions(
	user, project, anaconda_token, token_name, gh
	)
	except Exception as e:
	if "DEBUG_ANACONDA_TOKENS" in os.environ:
	raise e
	else:
	err_msg = (
	"Failed to rotate token for %s/%s"
	" on github actions!"
	) % (user, project)
	failed = True
	raise RuntimeError(err_msg)
	except Exception as e:
	if "DEBUG_ANACONDA_TOKENS" in os.environ:
	raise e
	failed = True
	if failed:
	if err_msg:
	raise RuntimeError(err_msg)
	else:
	raise RuntimeError(
	(
	"Rotating the feedstock token in providers for %s/%s failed!"
	" Try the command locally with DEBUG_ANACONDA_TOKENS"
	" defined in the environment to investigate!"
	)
	% (user, project)
	)

[beckermr]

This CLI command / function is used in staged recipes and when we rotate tokens for cf-staging via the admin migrations. IDK where you got the idea that it may not be used, but it definitely is. I am going to close this issue since there is nothing to do here.

[beckermr]

Also this method is used in staged-recipes and so I don't understand how it is "seriously broken."

[ytausch]

This CLI command / function is used in staged recipes and when we rotate tokens for cf-staging via the admin migrations.

Thank you. I could confirm that it is indeed used here

IDK where you got the idea that it may not be used, but it definitely is.

From the fact that the name of the command is update-anaconda-token and this string not appearing anywhere else on GitHub. The aliases rotate-anaconda-token and update-binstar-token are also not used anywhere. Apparently I did not see that the very outdated alias rotate-binstar-token has 1 usage in cf-staging, so good that we clarified that.

I am going to close this issue since there is nothing to do here.
Also this method is used in staged-recipes and so I don't understand how it is "seriously broken."

I think we misunderstood each other here. With "seriously broken" I, a bit sloppily, referred to the fact that there are some very obvious code quality issues with this function:

1. There are 15 lines of code which are copy-pasted 6 times for each CI service
2. The outer try-expect block is useless since it just repeats the copy-pasted inner except clauses
3. It doesn't make any sense to me that there is another if block raising exceptions again
4. All of this contributes to the exception flow being very obscure and redundant which should be avoided.

I would appreciate if you could reopen this issue. @beckermr

[beckermr]

IIRC the various try except statements catch errors and mask tokens at various steps. I'd take a closer look to make sure you understand the logic before touching the code.

[ytausch]

I don't think it's very productive to go into more specifics here since this is not a Pull Request, just a note about improvable code quality. But since I want this to be reopened, here is some additional opinions:

1. The copy-pasted lines of code can be made more generic. To illustrate what I mean, here is how ChatGPT would do it:

def rotate_token_processing(service_name, service_func, *args):
    try:
        service_func(*args)
    except Exception as e:
        if "DEBUG_ANACONDA_TOKENS" in os.environ:
            raise e
        else:
            err_msg = f"Failed to rotate token for {args[0]}/{args[1]} on {service_name}!"
            raise RuntimeError(err_msg)


services = {
    'circle': [rotate_token_in_circle, [user, project, anaconda_token, token_name]],
    'drone': [rotate_token_in_drone, [user, project, anaconda_token, token_name, drone_endpoint]],
    'travis': [rotate_token_in_travis, [user, project, feedstock_config_path, anaconda_token, token_name]],
    'azure': [rotate_token_in_azure, [user, project, anaconda_token, token_name]],
    'appveyor': [rotate_token_in_appveyor, [feedstock_config_path, anaconda_token, token_name]],
    'github_actions': [rotate_token_in_github_actions, [user, project, anaconda_token, token_name, gh]]
}

for service in [(service_name, service_funcs) for service_name, service_funcs in services.items() if service_name]:
    rotate_token_processing(service[0], service[1][0], *service[1][1])

This is not a concrete proposal since this piece of code behaves a little bit differently in some edge cases and still has some readability problems. But it has no redundancy and is a good starting point for illustrating how a better version of this could look like.

2. This line never has any effect. Why? Because the outer try-except block will only catch exceptions that either have "DEBUG_ANACONDA_TOKENS" in os.environ or failed = True. Setting failed to True again is pointless.
3. This else-clause can never be reached. Line 192 can only be reached if failed is true. If failed is True. err_msg is truthy because line 189 never changes failed from False to True (see above) and otherwise, failed is only set to True if err_msg is set.
4. There are no early returns but there should be.

Please reopen this issue.

[ytausch]

@beckermr I am happy to contribute a PR for this but it would be very helpful to reopen this so I can track this issue better for myself. Not sure if I made it clear that this came up in the ruff PR #1919 and I just wanted to move the discussion to here because it is unrelated to ruff.