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

Codecov upload is attempted if CODECOV_TOKEN is set, regardless of whether --codecov is provided #2

Open
adamcunnington-gdt opened this issue Oct 26, 2021 · 6 comments

Comments

@adamcunnington-gdt
Copy link

adamcunnington-gdt commented Oct 26, 2021

@Daverball I've ran into an irritating bug.

If the environment variable, CODECOV_TOKEN is set, even to an empty string, this package will cause upload to ALWAYS be attempted (resulting in failure when the token is not set).

My expectation is that this should only happen if I explicitly pass --codecov (or maybe --codecov-token) as a CLI option. I do not expect the upload to be attempted if I don't pass this.

Right now, the only way I can avoid upload is by explicitly unsetting the environment variable (even setting to empty string is not enough)!

@Daverball
Copy link
Member

@adamcunnington-gdt I see what you are talking about.

It is not actually attempting to upload anything. But the validation of parameters happens always, since they can be provided in any order and the defaults of those parameters are set to the contents of the environment variables. The way this happens is determined by pytest, so as a plugin I do not have that much control over it.

This is a bit annoying when you didn't set --codecov. However, if I delay validation of environment variables to the end, the behavior is now no longer consistent with providing the arguments to the command. What I would like to be able to do is to silently fail the arguments, unless --codecov is provided as well and only raise the error at the very end once all arguments have been parsed, although I'm not sure argparse can handle such a case.

What I can do either way is treat an empty string in an environment variable the same as an unset environment variable. I will investigate what can be done in the other cases.

Maybe I will decide that it is fine to delay validation of passed arguments and just print an error message to the log, while still running the tests, however that is maybe not what you want in an CI environment, where you're not going to check the command output. But then again the upload can still fail, without the command outputting an error code currently, since it is a bit tricky to fail a pytest run, after all the tests have already passed. I know the cov plugin uses some hacky way to accomplish this, but I haven't had the time to figure out if I can match that behavior.

@adamcunnington-gdt
Copy link
Author

adamcunnington-gdt commented Oct 27, 2021

@Daverball thanks for the response.

I think treating an empty string the same as unset is fine - and common practice (this would certainly be a super fast win please!).

Another alternative (if you don't wish to delay validation as you mention) is to allow me to provide some explicit option to actively supress uploading, e.g. --no-codecov. You can then hang behaviour off that.

@Daverball
Copy link
Member

@adamcunnington-gdt The error is generated during the argument parsing, so there isn't really anything to disable at this point. It would not actually upload anything if the provided token were valid, since the actual upload step does require explicitly setting --codecov. It just complains about the invalidity of the token, since validation happens during argument parsing. It errors out before pytest does anything note worthy at all.

@adamcunnington-gdt
Copy link
Author

Ah ok - are you happy to implement a small change that treats empty string as unset at least?

@Daverball
Copy link
Member

Absolutely.

@Daverball
Copy link
Member

Implemented treatment of empty environment variables via d9bcd28

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants