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

Remove CERTBOT_NO_PIN tests #9634

Merged
merged 1 commit into from
Mar 28, 2023
Merged

Remove CERTBOT_NO_PIN tests #9634

merged 1 commit into from
Mar 28, 2023

Conversation

bmw
Copy link
Member

@bmw bmw commented Mar 27, 2023

Adrien and I added this is in #6590 in response to #6582 which I wrote. I now personally think these tests are way more trouble than they're worth.

In almost all cases, the versions pinned in tools/requirements.txt are used. The two exceptions to this that come to mind are users using OS packages and pip. In the former, the version of our dependencies is picked by the OS and do not change much on most systems. As for pip, we only "support it on a best effort basis".

Even for pip users, I'm not convinced this buys us much other than frequent test failures. We have our tests configured to error on all Python warnings and we regularly update tools/requirements.txt. Due to that, assuming our dependencies follow normal conventions, we should have a chance to fix things in response to planned API changes long before they make their way to our users. I do not think it is necessary for our tests to break immediately after an API is deprecated.

I think almost all other failures due to these tests are caused by upstream bugs. In my experience, almost all of them will sort themselves out pretty quickly. I think that responding to those that are not or planned API changes we somehow missed can be addressed when tools/requirements.txt is updated or when someone opens an issue. I personally don't think blocking releases or causing our nightly tests to fail is at all worth it here. I think removing this frequent cause of test failures makes things just a little bit easier for Certbot devs without costing us much of anything.

@alexzorin
Copy link
Collaborator

alexzorin commented Mar 27, 2023

I appreciate the existence of this test because it gives advanced warning of issues that our real users (on pip) may be encountering. I am interested in seeing notifications, reading the cause and not being taken by surprise when it comes up on the forum - even if it's a no-op a lot of the time.

Is there a compromise that can be found? For example, preventing it from failing the test suite, or having a more subdued notification, like to the GitHub Notifications channel? Not erroring on deprecated APIs?

If it causes our test configuration to have a lot of divergences to enable this though, I guess I'm OK with deleting it.

@bmw
Copy link
Member Author

bmw commented Mar 27, 2023

Thanks for sharing your opinion! To push back just a little bit, has this test prevented you from being surprised on the forum (m)any times before?

I'm open to doing something more complex than just deleting this test, but I'd personally prefer if it had demonstrated some real value in the 4+ years it has been around that I was previously unaware of rather than having only done so in theory.

@alexzorin
Copy link
Collaborator

I guess it hasn't! I suppose I am reassured by the theoretical comfort of it.

Let's delete it.

@bmw
Copy link
Member Author

bmw commented Mar 28, 2023

Thanks! I'd be happy to reconsider if we find a reason to do so in the future.

@bmw bmw merged commit 208ef4e into master Mar 28, 2023
@bmw bmw deleted the test-no-no-pin branch March 28, 2023 00:01
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

Successfully merging this pull request may close these issues.

None yet

2 participants