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.

Sign up for GitHub

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

Jump to bottom

chore: Remove CROSS_REFERENCES feature flag #21815

Merged
merged 4 commits into from
Oct 21, 2022
Merged

chore: Remove CROSS_REFERENCES feature flag #21815

merged 4 commits into from
Oct 21, 2022

Conversation

geido
Copy link
Member

@geido geido commented Oct 14, 2022

SUMMARY

This PR removes the CROSS_REFERENCES feature flag as the feature will be enabled by default.

Approval from QA is required before merging this PR

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

N.A

TESTING INSTRUCTIONS

All the tests should pass

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

@geido geido requested a review from jinghua-qa October 14, 2022 12:12
@geido geido added the need:qa-review Requires QA review label Oct 14, 2022
@codecov
Copy link

codecov bot commented Oct 14, 2022 •
edited
Loading

Codecov Report

Merging #21815 (da3399b) into master (383dc29) will decrease coverage by 0.00%.
The diff coverage is 50.00%.

@@            Coverage Diff             @@
##           master   #21815      +/-   ##
==========================================
- Coverage   66.93%   66.93%   -0.01%     
==========================================
  Files        1805     1805              
  Lines       69066    69058       -8     
  Branches     7369     7366       -3     
==========================================
- Hits        46227    46221       -6     
+ Misses      20932    20931       -1     
+ Partials     1907     1906       -1
Flag Coverage Δ
hive 52.92% <ø> (ø)
javascript 53.33% <50.00%> (-0.01%) ⬇️
mysql 78.35% <ø> (ø)
postgres 78.41% <ø> (ø)
presto 52.82% <ø> (ø)
python 81.47% <ø> (ø)
sqlite 76.90% <ø> (ø)
unit 51.06% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...ackages/superset-ui-core/src/utils/featureFlags.ts 100.00% <ø> (ø)
...rc/explore/components/ExploreChartHeader/index.jsx 55.76% <0.00%> (ø)
...mponents/useExploreAdditionalActionsMenu/index.jsx 63.41% <ø> (+0.76%) ⬆️
superset/config.py 91.63% <ø> (ø)
...perset-frontend/src/views/CRUD/chart/ChartList.tsx 55.03% <50.00%> (-1.58%) ⬇️
...ExploreAdditionalActionsMenu/DashboardsSubMenu.tsx 83.33% <100.00%> (ø)
...ntend/src/components/MetadataBar/ContentConfig.tsx 85.00% <0.00%> (-5.00%) ⬇️
...ols/MetricControl/AdhocMetricEditPopover/index.jsx 75.53% <0.00%> (+1.06%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@michael-s-molina
Copy link
Member

We also need to remove the flag from RESOURCES/FEATURE_FLAGS.md.

@jinghua-qa
Copy link
Member

/testenv up

@github-actions
Copy link
Contributor

@jinghua-qa Ephemeral environment spinning up at http://34.218.244.229:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

@apache apache deleted a comment from github-actions bot Oct 17, 2022
@apache apache deleted a comment from jinghua-qa Oct 17, 2022
@geido
Copy link
Member Author

geido commented Oct 17, 2022

/testenv up

@github-actions
Copy link
Contributor

@geido Ephemeral environment spinning up at http://35.85.138.179:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

@geido
Copy link
Member Author

geido commented Oct 18, 2022 •
edited
Loading

Feature Acceptance Test passed for crud views. I will be merging this PR once the FAT for Explore is also confirmed

john-bodley
john-bodley reviewed Oct 18, 2022
View reviewed changes
superset/config.py
@@ -466,7 +466,6 @@ def _try_json_readsha(filepath: str, length: int) -> Optional[str]:
"EMBEDDABLE_CHARTS": True,
"DRILL_TO_DETAIL": False,
"DATAPANEL_CLOSED_BY_DEFAULT": False,
"CROSS_REFERENCES": False,
Copy link
Member

Choose a reason for hiding this comment

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

Is the preferred strategy to first enable the CROSS_REFERENCES feature by default and then deprecate the feature in version 3.0?

Also SIP-57 changing the default or removing a feature flag is deemed a breaking change and thus the appropriate actions need to be undertaken.

Copy link
Member

Choose a reason for hiding this comment

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

This one is a little funny, because the feature flag never went out in a version, and will not. So adding and removing it between published versions was perceived as a non-issue. We were hoping to implement a good means of launching darkly using this sort of ephemeral feature flag in lieu of a giant feature branch. Perhaps if we need to do this again, we can add a new category of "Pending release" or something to FEATURE_FLAGS.md to explain these?

I'll bring this up in Town Hall, and be seeking more opinions in general, but hopefully we can let this one through since the FF was never released or intended to be released.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the context @rusackas. This SGTM.

@jinghua-qa
Copy link
Member

found 1 issue that the search bar in 'dashboards added to' in explore, it is not able to accept user typing for space
Screen Shot 2022-10-18 at 11 09 42 AM

@pull-request-size pull-request-size bot added size/L and removed size/M labels Oct 19, 2022
@jinghua-qa
Copy link
Member

/testenv up

@github-actions
Copy link
Contributor

@jinghua-qa Ephemeral environment spinning up at http://35.86.76.39:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

@jinghua-qa
Copy link
Member

Validated in the recent ephemeral env that the search issue is fixed

jinghua-qa
jinghua-qa approved these changes Oct 20, 2022
View reviewed changes
Copy link
Member

@jinghua-qa jinghua-qa left a comment

Choose a reason for hiding this comment

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

LGTM

michael-s-molina
michael-s-molina approved these changes Oct 20, 2022
View reviewed changes
Copy link
Member

@michael-s-molina michael-s-molina left a comment

Choose a reason for hiding this comment

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

LGTM

@geido geido merged commit c2834cc into master Oct 21, 2022
@github-actions
Copy link
Contributor

Ephemeral environment shutdown and build artifacts deleted.

@rusackas rusackas deleted the chore/rm-crossreferences-feat-flag branch October 25, 2022 13:55
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 2.1.0 and removed 🚢 2.1.3 labels Mar 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@john-bodley john-bodley john-bodley left review comments

@rusackas rusackas rusackas left review comments

@jinghua-qa jinghua-qa jinghua-qa approved these changes

@michael-s-molina michael-s-molina michael-s-molina approved these changes

Assignees
No one assigned
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels need:qa-review Requires QA review size/L 🚢 2.1.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@geido @michael-s-molina @jinghua-qa @rusackas @john-bodley @mistercrunch