You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2021/02/17 18:04:38 UTC

[GitHub] [superset] hughhhh opened a new pull request #13182: feat: Move SQLAlchemy url reference to config

hughhhh opened a new pull request #13182:
URL: https://github.com/apache/superset/pull/13182


   ### SUMMARY
   <!--- Describe the change below, including rationale and design decisions -->
   Giving the ability for us at preset, to set the value for the sqlalchemy doc when running in our env
   
   ### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
   <!--- Skip this if not applicable -->
   
   ### TEST PLAN
   <!--- What steps should be taken to verify the changes -->
   
   ### ADDITIONAL INFORMATION
   <!--- Check any relevant boxes with "x" -->
   <!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
   - [ ] Has associated issue:
   - [ ] Changes UI
   - [ ] Requires DB Migration.
   - [ ] Confirm DB Migration upgrade and downgrade tested.
   - [ ] Introduces new feature or API
   - [ ] Removes existing feature or API
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] eschutho commented on a change in pull request #13182: feat: Move SQLAlchemy url reference to config

Posted by GitBox <gi...@apache.org>.
eschutho commented on a change in pull request #13182:
URL: https://github.com/apache/superset/pull/13182#discussion_r577996619



##########
File path: superset-frontend/src/views/CRUD/data/database/DatabaseModal.tsx
##########
@@ -402,7 +419,7 @@ const DatabaseModal: FunctionComponent<DatabaseModalProps> = ({
             <div className="helper">
               {t('Refer to the ')}
               <a
-                href="https://docs.sqlalchemy.org/en/rel_1_2/core/engines.html#"
+                href={conf?.SQLALCHEMY_DOCS_URL ?? DEFAULT_SQLALCHEMY_DOCS_URL}

Review comment:
       the text for the link. "refer to the sqlalchemy docs...". It's in the ticket, too.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] codecov-io edited a comment on pull request #13182: feat: Move SQLAlchemy url reference to config

Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #13182:
URL: https://github.com/apache/superset/pull/13182#issuecomment-780850136


   # [Codecov](https://codecov.io/gh/apache/superset/pull/13182?src=pr&el=h1) Report
   > Merging [#13182](https://codecov.io/gh/apache/superset/pull/13182?src=pr&el=desc) (6545601) into [master](https://codecov.io/gh/apache/superset/commit/2ce79823dfad61bce6196fcacd56a844f44818c0?el=desc) (2ce7982) will **increase** coverage by `11.94%`.
   > The diff coverage is `48.72%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/superset/pull/13182/graphs/tree.svg?width=650&height=150&src=pr&token=KsB0fHcx6l)](https://codecov.io/gh/apache/superset/pull/13182?src=pr&el=tree)
   
   ```diff
   @@             Coverage Diff             @@
   ##           master   #13182       +/-   ##
   ===========================================
   + Coverage   53.06%   65.00%   +11.94%     
   ===========================================
     Files         489     1043      +554     
     Lines       17314    49394    +32080     
     Branches     4482     5338      +856     
   ===========================================
   + Hits         9187    32109    +22922     
   - Misses       8127    17069     +8942     
   - Partials        0      216      +216     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | cypress | `?` | |
   | javascript | `62.38% <46.40%> (?)` | |
   | python | `66.83% <51.45%> (?)` | |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   | [Impacted Files](https://codecov.io/gh/apache/superset/pull/13182?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [...end/src/SqlLab/components/RunQueryActionButton.tsx](https://codecov.io/gh/apache/superset/pull/13182/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1J1blF1ZXJ5QWN0aW9uQnV0dG9uLnRzeA==) | `64.28% <ø> (+11.50%)` | :arrow_up: |
   | [superset-frontend/src/chart/ChartContainer.jsx](https://codecov.io/gh/apache/superset/pull/13182/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NoYXJ0L0NoYXJ0Q29udGFpbmVyLmpzeA==) | `100.00% <ø> (ø)` | |
   | [superset-frontend/src/chart/ChartRenderer.jsx](https://codecov.io/gh/apache/superset/pull/13182/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NoYXJ0L0NoYXJ0UmVuZGVyZXIuanN4) | `39.18% <0.00%> (-37.53%)` | :arrow_down: |
   | [...perset-frontend/src/common/components/Dropdown.tsx](https://codecov.io/gh/apache/superset/pull/13182/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbW1vbi9jb21wb25lbnRzL0Ryb3Bkb3duLnRzeA==) | `54.76% <ø> (+4.76%)` | :arrow_up: |
   | [...ontend/src/components/ListViewCard/ImageLoader.tsx](https://codecov.io/gh/apache/superset/pull/13182/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvTGlzdFZpZXdDYXJkL0ltYWdlTG9hZGVyLnRzeA==) | `86.36% <0.00%> (+11.36%)` | :arrow_up: |
   | [...set-frontend/src/components/URLShortLinkButton.jsx](https://codecov.io/gh/apache/superset/pull/13182/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvVVJMU2hvcnRMaW5rQnV0dG9uLmpzeA==) | `100.00% <ø> (ø)` | |
   | [.../src/components/dataViewCommon/TableCollection.tsx](https://codecov.io/gh/apache/superset/pull/13182/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvZGF0YVZpZXdDb21tb24vVGFibGVDb2xsZWN0aW9uLnRzeA==) | `100.00% <ø> (+2.12%)` | :arrow_up: |
   | [...et-frontend/src/dashboard/actions/sliceEntities.js](https://codecov.io/gh/apache/superset/pull/13182/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9hY3Rpb25zL3NsaWNlRW50aXRpZXMuanM=) | `12.90% <ø> (-58.07%)` | :arrow_down: |
   | [...src/dashboard/components/HeaderActionsDropdown.jsx](https://codecov.io/gh/apache/superset/pull/13182/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL0hlYWRlckFjdGlvbnNEcm9wZG93bi5qc3g=) | `54.79% <ø> (-12.78%)` | :arrow_down: |
   | [...end/src/dashboard/components/StickyVerticalBar.tsx](https://codecov.io/gh/apache/superset/pull/13182/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL1N0aWNreVZlcnRpY2FsQmFyLnRzeA==) | `50.00% <ø> (-50.00%)` | :arrow_down: |
   | ... and [1050 more](https://codecov.io/gh/apache/superset/pull/13182/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/13182?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/13182?src=pr&el=footer). Last update [6e09156...6545601](https://codecov.io/gh/apache/superset/pull/13182?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] eschutho commented on a change in pull request #13182: feat: Move SQLAlchemy url reference to config

Posted by GitBox <gi...@apache.org>.
eschutho commented on a change in pull request #13182:
URL: https://github.com/apache/superset/pull/13182#discussion_r577911163



##########
File path: superset/config.py
##########
@@ -1092,6 +1092,9 @@ class CeleryConfig:  # pylint: disable=too-few-public-methods
 # It will get executed each time when user open a chart's explore view.
 DATASET_HEALTH_CHECK = None
 
+# SQLAlchema link doc reference

Review comment:
       typo




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] hughhhh commented on a change in pull request #13182: feat: Move SQLAlchemy url reference to config

Posted by GitBox <gi...@apache.org>.
hughhhh commented on a change in pull request #13182:
URL: https://github.com/apache/superset/pull/13182#discussion_r577928673



##########
File path: superset-frontend/src/views/CRUD/data/database/DatabaseModal.tsx
##########
@@ -39,7 +40,19 @@ interface DatabaseModalProps {
   database?: DatabaseObject | null; // If included, will go into edit mode
 }
 
+// todo: define common type fully in types file
+interface RootState {
+  common: {
+    conf: {
+      SQLALCHEMY_DOCS_URL: string;
+    };
+  };
+  messageToast: Array<Object>;
+}
+
 const DEFAULT_TAB_KEY = '1';
+const DEFAULT_SQLALCHEMY_DOCS_URL =
+  'https://docs.sqlalchemy.org/en/rel_1_2/core/engines.html#';

Review comment:
       The default i'm using since the unit test aren't playing nice, i couldn't figure out how to get the `wrappingComponent must render its children!`




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] betodealmeida commented on a change in pull request #13182: feat: Move SQLAlchemy url reference to config

Posted by GitBox <gi...@apache.org>.
betodealmeida commented on a change in pull request #13182:
URL: https://github.com/apache/superset/pull/13182#discussion_r578022129



##########
File path: superset/config.py
##########
@@ -1092,6 +1092,9 @@ class CeleryConfig:  # pylint: disable=too-few-public-methods
 # It will get executed each time when user open a chart's explore view.
 DATASET_HEALTH_CHECK = None
 
+# SQLalchemy link doc reference
+SQLALCHEMY_DOCS_URL = "https://docs.sqlalchemy.org/en/13/core/engines.html"

Review comment:
       Noice!




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] eschutho commented on a change in pull request #13182: feat: Move SQLAlchemy url reference to config

Posted by GitBox <gi...@apache.org>.
eschutho commented on a change in pull request #13182:
URL: https://github.com/apache/superset/pull/13182#discussion_r577919312



##########
File path: superset-frontend/src/views/CRUD/data/database/DatabaseModal.tsx
##########
@@ -402,7 +419,7 @@ const DatabaseModal: FunctionComponent<DatabaseModalProps> = ({
             <div className="helper">
               {t('Refer to the ')}
               <a
-                href="https://docs.sqlalchemy.org/en/rel_1_2/core/engines.html#"
+                href={conf?.SQLALCHEMY_DOCS_URL ?? DEFAULT_SQLALCHEMY_DOCS_URL}

Review comment:
       do we want to also update the string text below?




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] hughhhh closed pull request #13182: feat: Move SQLAlchemy url reference to config

Posted by GitBox <gi...@apache.org>.
hughhhh closed pull request #13182:
URL: https://github.com/apache/superset/pull/13182


   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] codecov-io commented on pull request #13182: feat: Move SQLAlchemy url reference to config

Posted by GitBox <gi...@apache.org>.
codecov-io commented on pull request #13182:
URL: https://github.com/apache/superset/pull/13182#issuecomment-780850136


   # [Codecov](https://codecov.io/gh/apache/superset/pull/13182?src=pr&el=h1) Report
   > Merging [#13182](https://codecov.io/gh/apache/superset/pull/13182?src=pr&el=desc) (6545601) into [master](https://codecov.io/gh/apache/superset/commit/2ce79823dfad61bce6196fcacd56a844f44818c0?el=desc) (2ce7982) will **increase** coverage by `9.32%`.
   > The diff coverage is `46.40%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/superset/pull/13182/graphs/tree.svg?width=650&height=150&src=pr&token=KsB0fHcx6l)](https://codecov.io/gh/apache/superset/pull/13182?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master   #13182      +/-   ##
   ==========================================
   + Coverage   53.06%   62.38%   +9.32%     
   ==========================================
     Files         489      551      +62     
     Lines       17314    20353    +3039     
     Branches     4482     5338     +856     
   ==========================================
   + Hits         9187    12698    +3511     
   + Misses       8127     7439     -688     
   - Partials        0      216     +216     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | cypress | `?` | |
   | javascript | `62.38% <46.40%> (?)` | |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   | [Impacted Files](https://codecov.io/gh/apache/superset/pull/13182?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [...end/src/SqlLab/components/RunQueryActionButton.tsx](https://codecov.io/gh/apache/superset/pull/13182/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1J1blF1ZXJ5QWN0aW9uQnV0dG9uLnRzeA==) | `64.28% <ø> (+11.50%)` | :arrow_up: |
   | [superset-frontend/src/chart/ChartContainer.jsx](https://codecov.io/gh/apache/superset/pull/13182/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NoYXJ0L0NoYXJ0Q29udGFpbmVyLmpzeA==) | `100.00% <ø> (ø)` | |
   | [superset-frontend/src/chart/ChartRenderer.jsx](https://codecov.io/gh/apache/superset/pull/13182/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NoYXJ0L0NoYXJ0UmVuZGVyZXIuanN4) | `39.18% <0.00%> (-37.53%)` | :arrow_down: |
   | [...perset-frontend/src/common/components/Dropdown.tsx](https://codecov.io/gh/apache/superset/pull/13182/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbW1vbi9jb21wb25lbnRzL0Ryb3Bkb3duLnRzeA==) | `54.76% <ø> (+4.76%)` | :arrow_up: |
   | [...ontend/src/components/ListViewCard/ImageLoader.tsx](https://codecov.io/gh/apache/superset/pull/13182/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvTGlzdFZpZXdDYXJkL0ltYWdlTG9hZGVyLnRzeA==) | `86.36% <0.00%> (+11.36%)` | :arrow_up: |
   | [...set-frontend/src/components/URLShortLinkButton.jsx](https://codecov.io/gh/apache/superset/pull/13182/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvVVJMU2hvcnRMaW5rQnV0dG9uLmpzeA==) | `100.00% <ø> (ø)` | |
   | [.../src/components/dataViewCommon/TableCollection.tsx](https://codecov.io/gh/apache/superset/pull/13182/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvZGF0YVZpZXdDb21tb24vVGFibGVDb2xsZWN0aW9uLnRzeA==) | `100.00% <ø> (+2.12%)` | :arrow_up: |
   | [...et-frontend/src/dashboard/actions/sliceEntities.js](https://codecov.io/gh/apache/superset/pull/13182/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9hY3Rpb25zL3NsaWNlRW50aXRpZXMuanM=) | `12.90% <ø> (-58.07%)` | :arrow_down: |
   | [...src/dashboard/components/HeaderActionsDropdown.jsx](https://codecov.io/gh/apache/superset/pull/13182/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL0hlYWRlckFjdGlvbnNEcm9wZG93bi5qc3g=) | `54.79% <ø> (-12.78%)` | :arrow_down: |
   | [...end/src/dashboard/components/StickyVerticalBar.tsx](https://codecov.io/gh/apache/superset/pull/13182/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL1N0aWNreVZlcnRpY2FsQmFyLnRzeA==) | `50.00% <ø> (-50.00%)` | :arrow_down: |
   | ... and [533 more](https://codecov.io/gh/apache/superset/pull/13182/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/13182?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/13182?src=pr&el=footer). Last update [6e09156...6545601](https://codecov.io/gh/apache/superset/pull/13182?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] eschutho commented on a change in pull request #13182: feat: Move SQLAlchemy url reference to config

Posted by GitBox <gi...@apache.org>.
eschutho commented on a change in pull request #13182:
URL: https://github.com/apache/superset/pull/13182#discussion_r577910585



##########
File path: superset-frontend/src/views/CRUD/data/database/DatabaseModal.tsx
##########
@@ -39,7 +40,19 @@ interface DatabaseModalProps {
   database?: DatabaseObject | null; // If included, will go into edit mode
 }
 
+// todo: define common type fully in types file
+interface RootState {
+  common: {
+    conf: {
+      SQLALCHEMY_DOCS_URL: string;
+    };
+  };
+  messageToast: Array<Object>;
+}
+
 const DEFAULT_TAB_KEY = '1';
+const DEFAULT_SQLALCHEMY_DOCS_URL =
+  'https://docs.sqlalchemy.org/en/rel_1_2/core/engines.html#';

Review comment:
       do you want to put this in the default config file instead?

##########
File path: superset-frontend/src/views/CRUD/data/database/DatabaseModal.tsx
##########
@@ -132,6 +145,10 @@ const DatabaseModal: FunctionComponent<DatabaseModalProps> = ({
   const [db, setDB] = useState<DatabaseObject | null>(null);
   const [isHidden, setIsHidden] = useState<boolean>(true);
   const [tabKey, setTabKey] = useState<string>(DEFAULT_TAB_KEY);
+  const conf = useSelector((state: RootState) => {
+    console.log(state);

Review comment:
       console.log




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] hughhhh commented on a change in pull request #13182: feat: Move SQLAlchemy url reference to config

Posted by GitBox <gi...@apache.org>.
hughhhh commented on a change in pull request #13182:
URL: https://github.com/apache/superset/pull/13182#discussion_r577933760



##########
File path: superset-frontend/src/views/CRUD/data/database/DatabaseModal.tsx
##########
@@ -402,7 +419,7 @@ const DatabaseModal: FunctionComponent<DatabaseModalProps> = ({
             <div className="helper">
               {t('Refer to the ')}
               <a
-                href="https://docs.sqlalchemy.org/en/rel_1_2/core/engines.html#"
+                href={conf?.SQLALCHEMY_DOCS_URL ?? DEFAULT_SQLALCHEMY_DOCS_URL}

Review comment:
       what string text are your referencing?




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] eschutho commented on a change in pull request #13182: feat: Move SQLAlchemy url reference to config

Posted by GitBox <gi...@apache.org>.
eschutho commented on a change in pull request #13182:
URL: https://github.com/apache/superset/pull/13182#discussion_r577918741



##########
File path: superset/config.py
##########
@@ -1092,6 +1092,9 @@ class CeleryConfig:  # pylint: disable=too-few-public-methods
 # It will get executed each time when user open a chart's explore view.
 DATASET_HEALTH_CHECK = None
 
+# SQLAlchema link doc reference
+SQLALCHEMY_DOCS_URL = "https://docs.sqlalchemy.org/en/rel_1_2/core/engines.html#"

Review comment:
       tiny tiny nit, but prob don't need the # at the end.
   I suppose we should also update the link to `https://docs.sqlalchemy.org/en/13/core/engines.html` since we're on 1.3 now.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] eschutho commented on a change in pull request #13182: feat: Move SQLAlchemy url reference to config

Posted by GitBox <gi...@apache.org>.
eschutho commented on a change in pull request #13182:
URL: https://github.com/apache/superset/pull/13182#discussion_r577911975



##########
File path: superset-frontend/src/views/CRUD/data/database/DatabaseModal.tsx
##########
@@ -39,7 +40,19 @@ interface DatabaseModalProps {
   database?: DatabaseObject | null; // If included, will go into edit mode
 }
 
+// todo: define common type fully in types file
+interface RootState {
+  common: {
+    conf: {
+      SQLALCHEMY_DOCS_URL: string;
+    };
+  };
+  messageToast: Array<Object>;
+}
+
 const DEFAULT_TAB_KEY = '1';
+const DEFAULT_SQLALCHEMY_DOCS_URL =
+  'https://docs.sqlalchemy.org/en/rel_1_2/core/engines.html#';

Review comment:
       Actually I see you also have it there.. seems like unless someone overwrites it with an empty string in their own config, it should always exist and we shouldn't need a default here as well? 




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] eschutho commented on a change in pull request #13182: feat: Move SQLAlchemy url reference to config

Posted by GitBox <gi...@apache.org>.
eschutho commented on a change in pull request #13182:
URL: https://github.com/apache/superset/pull/13182#discussion_r577912389



##########
File path: superset-frontend/src/views/CRUD/data/database/DatabaseModal.tsx
##########
@@ -402,7 +419,7 @@ const DatabaseModal: FunctionComponent<DatabaseModalProps> = ({
             <div className="helper">
               {t('Refer to the ')}
               <a
-                href="https://docs.sqlalchemy.org/en/rel_1_2/core/engines.html#"
+                href={conf?.SQLALCHEMY_DOCS_URL ?? DEFAULT_SQLALCHEMY_DOCS_URL}

Review comment:
       per above, it seems that just conf.SQLALCHEMY_DOCS_URL should always have a value.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] hughhhh merged pull request #13182: feat: Move SQLAlchemy url reference to config

Posted by GitBox <gi...@apache.org>.
hughhhh merged pull request #13182:
URL: https://github.com/apache/superset/pull/13182


   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org