You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "Always-prog (via GitHub)" <gi...@apache.org> on 2023/01/27 00:01:53 UTC
[GitHub] [superset] Always-prog opened a new pull request, #22880: fix: moved alerts and reports default values to config
Always-prog opened a new pull request, #22880:
URL: https://github.com/apache/superset/pull/22880
<!---
Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
Example:
fix(dashboard): load charts correctly
-->
### SUMMARY
Typically, users do not verify the working timeout and default repetition for creating alerts or reports. As a result, if a Superset administrator wants to change the default values for these options, they need to modify the code. To address this issue, I submitted a pull request that moves these default values to the config file.
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Required feature flags:
- [x] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] 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
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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] villebro commented on pull request #22880: fix: moved alerts and reports default values to config
Posted by "villebro (via GitHub)" <gi...@apache.org>.
villebro commented on PR #22880:
URL: https://github.com/apache/superset/pull/22880#issuecomment-1408243435
> @villebro Hello!
> I fixed the passing test cases because the test cases were running without the python config. We still need to have default values in the code. However, if they are declared in config, they will be used.
> Can you check my code?
@Always-prog yeah, this is a bit of a problem when the frontend assets don't have context on regarding the python config values. You can probably either mock the values on the test (more work) or just add the same defaults on the frontend code path (less work). I'm ok with both.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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] Always-prog commented on pull request #22880: fix: moved alerts and reports default values to config
Posted by "Always-prog (via GitHub)" <gi...@apache.org>.
Always-prog commented on PR #22880:
URL: https://github.com/apache/superset/pull/22880#issuecomment-1408671511
> > @villebro Hello!
> > I fixed the passing test cases because the test cases were running without the python config. We still need to have default values in the code. However, if they are declared in config, they will be used.
> > Can you check my code?
>
> @Always-prog yeah, this is a bit of a problem when the frontend assets don't have context on regarding the python config values. You can probably either mock the values on the test (more work) or just add the same defaults on the frontend code path (less work). I'm ok with both, as in practice they won't affect this functionality.
I added default values in code.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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] Always-prog commented on pull request #22880: fix: moved alerts and reports default values to config
Posted by "Always-prog (via GitHub)" <gi...@apache.org>.
Always-prog commented on PR #22880:
URL: https://github.com/apache/superset/pull/22880#issuecomment-1422097986
On my fork all CI test has passed.
Maybe need to restart them @dpgaspar?
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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] Always-prog commented on pull request #22880: fix: moved alerts and reports default values to config
Posted by "Always-prog (via GitHub)" <gi...@apache.org>.
Always-prog commented on PR #22880:
URL: https://github.com/apache/superset/pull/22880#issuecomment-1415977155
Hello @villebro and @rusackas!
I have fixed the frontend tests and they are now passing. Could you please review my code once more?
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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] Always-prog commented on pull request #22880: fix: moved alerts and reports default values to config
Posted by "Always-prog (via GitHub)" <gi...@apache.org>.
Always-prog commented on PR #22880:
URL: https://github.com/apache/superset/pull/22880#issuecomment-1416036766
I need to create a new PR for the fix?
There are many commits that are not mine, due to rebase.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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[bot] commented on pull request #22880: fix: moved alerts and reports default values to config
Posted by codecov.
codecov[bot] commented on PR #22880:
URL: https://github.com/apache/superset/pull/22880#issuecomment-1405838683
# [Codecov](https://codecov.io/gh/apache/superset/pull/22880?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#22880](https://codecov.io/gh/apache/superset/pull/22880?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (55509c3) into [master](https://codecov.io/gh/apache/superset/commit/3e07de7f39a1e6ee9d0db43435ce4a7285003a79?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (3e07de7) will **decrease** coverage by `11.45%`.
> The diff coverage is `90.90%`.
```diff
@@ Coverage Diff @@
## master #22880 +/- ##
===========================================
- Coverage 67.28% 55.84% -11.45%
===========================================
Files 1877 1879 +2
Lines 72023 72094 +71
Branches 7897 7897
===========================================
- Hits 48461 40260 -8201
- Misses 21534 29806 +8272
Partials 2028 2028
```
| Flag | Coverage Δ | |
|---|---|---|
| hive | `?` | |
| mysql | `?` | |
| postgres | `?` | |
| presto | `52.53% <100.00%> (+0.04%)` | :arrow_up: |
| python | `57.99% <100.00%> (-23.90%)` | :arrow_down: |
| sqlite | `?` | |
| unit | `51.64% <100.00%> (+0.09%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/22880?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [superset/views/base.py](https://codecov.io/gh/apache/superset/pull/22880?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvdmlld3MvYmFzZS5weQ==) | `62.32% <ø> (-14.73%)` | :arrow_down: |
| [...frontend/src/views/CRUD/alert/AlertReportModal.tsx](https://codecov.io/gh/apache/superset/pull/22880?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0NSVUQvYWxlcnQvQWxlcnRSZXBvcnRNb2RhbC50c3g=) | `53.44% <87.50%> (ø)` | |
| [superset/config.py](https://codecov.io/gh/apache/superset/pull/22880?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvY29uZmlnLnB5) | `91.25% <100.00%> (-0.52%)` | :arrow_down: |
| [superset/utils/dashboard\_import\_export.py](https://codecov.io/gh/apache/superset/pull/22880?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvdXRpbHMvZGFzaGJvYXJkX2ltcG9ydF9leHBvcnQucHk=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [superset/tags/core.py](https://codecov.io/gh/apache/superset/pull/22880?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvdGFncy9jb3JlLnB5) | `4.54% <0.00%> (-95.46%)` | :arrow_down: |
| [superset/key\_value/commands/update.py](https://codecov.io/gh/apache/superset/pull/22880?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQva2V5X3ZhbHVlL2NvbW1hbmRzL3VwZGF0ZS5weQ==) | `0.00% <0.00%> (-90.91%)` | :arrow_down: |
| [superset/key\_value/commands/delete.py](https://codecov.io/gh/apache/superset/pull/22880?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQva2V5X3ZhbHVlL2NvbW1hbmRzL2RlbGV0ZS5weQ==) | `0.00% <0.00%> (-87.88%)` | :arrow_down: |
| [superset/db\_engines/hive.py](https://codecov.io/gh/apache/superset/pull/22880?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGJfZW5naW5lcy9oaXZlLnB5) | `0.00% <0.00%> (-85.19%)` | :arrow_down: |
| [superset/key\_value/commands/delete\_expired.py](https://codecov.io/gh/apache/superset/pull/22880?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQva2V5X3ZhbHVlL2NvbW1hbmRzL2RlbGV0ZV9leHBpcmVkLnB5) | `0.00% <0.00%> (-84.00%)` | :arrow_down: |
| [superset/dashboards/commands/importers/v0.py](https://codecov.io/gh/apache/superset/pull/22880?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGFzaGJvYXJkcy9jb21tYW5kcy9pbXBvcnRlcnMvdjAucHk=) | `15.62% <0.00%> (-76.25%)` | :arrow_down: |
| ... and [299 more](https://codecov.io/gh/apache/superset/pull/22880?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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] Always-prog commented on a diff in pull request #22880: fix: moved alerts and reports default values to config
Posted by "Always-prog (via GitHub)" <gi...@apache.org>.
Always-prog commented on code in PR #22880:
URL: https://github.com/apache/superset/pull/22880#discussion_r1088719881
##########
superset-frontend/src/views/CRUD/alert/AlertReportModal.tsx:
##########
@@ -512,6 +495,28 @@ const AlertReportModal: FunctionComponent<AlertReportModalProps> = ({
);
};
+ const {
+ ALERT_REPORTS_DEFAULT_WORKING_TIMEOUT,
+ ALERT_REPORTS_DEFAULT_CRON_VALUE,
+ ALERT_REPORTS_DEFAULT_RETENTION,
+ } = useSelector<any, AlertsReportsConfig>(state => state.common.conf);
+
+ const DEFAULT_ALERT = {
Review Comment:
Thanks for soon reviewing!
Renamed.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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] Always-prog commented on pull request #22880: fix: moved alerts and reports default values to config
Posted by "Always-prog (via GitHub)" <gi...@apache.org>.
Always-prog commented on PR #22880:
URL: https://github.com/apache/superset/pull/22880#issuecomment-1436532533
@dpgaspar Hello!
Can you please review py PR again?
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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] Always-prog commented on pull request #22880: fix: moved alerts and reports default values to config
Posted by "Always-prog (via GitHub)" <gi...@apache.org>.
Always-prog commented on PR #22880:
URL: https://github.com/apache/superset/pull/22880#issuecomment-1491877888
Thanks for approving!
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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] villebro commented on pull request #22880: fix: moved alerts and reports default values to config
Posted by "villebro (via GitHub)" <gi...@apache.org>.
villebro commented on PR #22880:
URL: https://github.com/apache/superset/pull/22880#issuecomment-1412413980
@Always-prog can you rebase the PR? That might help resolve that failed test.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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] Always-prog commented on pull request #22880: fix: moved alerts and reports default values to config
Posted by "Always-prog (via GitHub)" <gi...@apache.org>.
Always-prog commented on PR #22880:
URL: https://github.com/apache/superset/pull/22880#issuecomment-1421335672
> probably something went wrong with the rebase
I reverted commit with rebasing.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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] villebro merged pull request #22880: fix: moved alerts and reports default values to config
Posted by "villebro (via GitHub)" <gi...@apache.org>.
villebro merged PR #22880:
URL: https://github.com/apache/superset/pull/22880
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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] villebro commented on pull request #22880: fix: moved alerts and reports default values to config
Posted by "villebro (via GitHub)" <gi...@apache.org>.
villebro commented on PR #22880:
URL: https://github.com/apache/superset/pull/22880#issuecomment-1422909471
@dpgaspar I believe the rebasing problems have been resolved, can you take another look?
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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] Always-prog commented on pull request #22880: fix: moved alerts and reports default values to config
Posted by "Always-prog (via GitHub)" <gi...@apache.org>.
Always-prog commented on PR #22880:
URL: https://github.com/apache/superset/pull/22880#issuecomment-1407409264
@villebro Hello!
I fixed the passing test cases because the test cases were running without the python config. We still need to have default values in the code. However, if they are declared in config, they will be used.
Can you check my code?
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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] villebro commented on a diff in pull request #22880: fix: moved alerts and reports default values to config
Posted by "villebro (via GitHub)" <gi...@apache.org>.
villebro commented on code in PR #22880:
URL: https://github.com/apache/superset/pull/22880#discussion_r1088686351
##########
superset-frontend/src/views/CRUD/alert/AlertReportModal.tsx:
##########
@@ -512,6 +495,28 @@ const AlertReportModal: FunctionComponent<AlertReportModalProps> = ({
);
};
+ const {
+ ALERT_REPORTS_DEFAULT_WORKING_TIMEOUT,
+ ALERT_REPORTS_DEFAULT_CRON_VALUE,
+ ALERT_REPORTS_DEFAULT_RETENTION,
+ } = useSelector<any, AlertsReportsConfig>(state => state.common.conf);
+
+ const DEFAULT_ALERT = {
Review Comment:
nit: very minor detail, but as this is not technically a constant, it might be be worth considering renaming this to `defaultAlert`.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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