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 broken and undocumented "demo mode" feature #14601

Merged
merged 1 commit into from Mar 11, 2021

Conversation

ryanahamilton
Copy link
Contributor

After fixing and refactoring the "demo mode" feature in #14595, I feel that it would be more productive to replace that PR with this one that removes the feature instead.

  • It has very little documentation
  • It has been largely broken for an unknown period of time without any issue reports. I only encountered it because I wanted to refactor its implementation.
  • As illustrated by @ashb it doesn't do a really great job if someone were to capture a screenshot from remote demonstration.
  • As pointed out by @HaloKo4, only being able to activate it from the config doesn't make it a very convenient feature that can easily be enabled/disabled.

If this is a feature people would like to see, I think it would be better to see a new implementation proposed that addresses these shortcomings.

@ryanahamilton ryanahamilton requested a review from ashb as a code owner March 4, 2021 14:30
@boring-cyborg boring-cyborg bot added area:UI Related to UI/UX. For Frontend Developers. area:webserver Webserver related Issues labels Mar 4, 2021
@ashb
Copy link
Member

ashb commented Mar 7, 2021

I think given its been broken for a while without reports, and the ability to unblur via GANs we should likely remove i let's give people a bit of a heads up on the dev mailing list end find them say, 3 days to object/propose something else.

@github-actions github-actions bot added the full tests needed We need to run full set of tests for this PR to merge label Mar 7, 2021
@github-actions
Copy link

github-actions bot commented Mar 7, 2021

The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest master at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.

@bbovenzi
Copy link
Contributor

bbovenzi commented Mar 8, 2021

If it's not all that useful, then I'm in favor of cleaning up at least a bit of the FE code.

@xinbinhuang
Copy link
Contributor

I suggest that we remove it. I only came across this when I was digging around the codebase, and have never heard of people using or talking about it previously. Mostly, I have seen people just creating some dummy dags for illustration and presentation purpose..

@ryanahamilton
Copy link
Contributor Author

I put a call out to the Dev list as a gut check on this and did not receive any dissenting feedback. Going ahead and merging this PR (removing the "demo mode" feature).

Failed checks are unrelated.

@ryanahamilton ryanahamilton merged commit 3be1e19 into apache:master Mar 11, 2021
@ryanahamilton ryanahamilton deleted the remove_demo_mode branch March 11, 2021 16:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:UI Related to UI/UX. For Frontend Developers. area:webserver Webserver related Issues full tests needed We need to run full set of tests for this PR to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants