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.

https://github.com/conda-forge/conda-smithy/blob/9e4720fc497f45f45d7bd319a354c762992c530d/conda_smithy/anaconda_token_rotation.py#L34-L203

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