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

Feature flag: remove unused ones #10423

Merged
merged 19 commits into from
Jun 14, 2023
Merged

Conversation

humitos
Copy link
Member

@humitos humitos commented Jun 12, 2023

Follows what's described in #9779.

This initial PR only removes feature flags that are not used at all, or are enabled by default to all the projects. Note that each commit removes one and only one feature flag. This could be useful while reviewing.

Once we merge this PR, we we have to remove all the Feature rows from the db to avoid future confusions as I had to deal with today 😄

Next iteration will remove more feature flags that require communication to users or some internal discussion.

humitos added 11 commits June 12, 2023 16:10
We can safely remove this feature flag since it's marked as `default_true=True`
and `future_default_true=True` in both platforms. This means that it applies to
all the projects.
It was a failed attempt to make Sphinx to run fast.
It became a Django setting and it's integrated in the application now.
It's enabled by default to all the projects and it has been working fine.
This was an attempt to show debugging information. This can be achieved now with
`build.jobs` in case the user want to display this data.

Internally, we are storing some of this data in BuildData from `telemetry`
database.
We have been using this by default in all projects.
We have been using this for a long time and we are happy.
We removed this code completely and we are not doing this anymore.
It's not used. Only leftovers.
Only used by 1 projects. Its last build was 2 years ago.
We have been using this by default to everybody for months now and it's working
fine. It's safe to remove.
@humitos humitos requested a review from a team as a code owner June 12, 2023 15:56
@humitos humitos requested a review from benjaoming June 12, 2023 15:56
@humitos humitos requested a review from stsewd June 12, 2023 15:56
@benjaoming
Copy link
Contributor

This is super nice!! The shear amount of feature flags is pretty uphill to look at as someone trying to understand how things work.

Once we merge this PR, we we have to remove all the Feature rows from the db to avoid future confusions as I had to deal with today smile

I was asking @stsewd about the same last week :)

Copy link
Contributor

@benjaoming benjaoming left a comment

Choose a reason for hiding this comment

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

Yes, it was tricky to review :) Some Feature flags were always on and some feature flags were always off... so when the flag is removed, we have to check that conditionals are restored with the correct assumption :)

I looked at the checks and they seem like Prospector bugs. I think we should be safe and that Prospector and several test cases would fail if an unreferenced feature flag was left.

Tests need to be updated. But once they work, everything should be fine 👍

readthedocs/builds/storage.py Show resolved Hide resolved
readthedocs/projects/tasks/builds.py Show resolved Hide resolved
@humitos
Copy link
Member Author

humitos commented Jun 13, 2023

I updated this PR to fix all the tests and remove the leftovers that I found. I the tests pass, we can merge it.

@benjaoming
Copy link
Contributor

Have you seen the current test errors (network errors) in other PRs? I don't think that I've seen it recently, so just wondering that i could be something related to these changes.

@humitos
Copy link
Member Author

humitos commented Jun 14, 2023

Yeah, it was a missing mock.

@humitos humitos merged commit 306194f into main Jun 14, 2023
@humitos humitos deleted the humitos/remove-old-feature-flags branch June 14, 2023 12:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants