[BUGFIX] Enable SlackNotificationAction variable substitution
#10443
Conversation
✅ Deploy Preview for niobium-lead-7998 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## develop #10443 +/- ##
========================================
Coverage 80.08% 80.09%
========================================
Files 461 461
Lines 39979 39990 +11
========================================
+ Hits 32019 32030 +11
Misses 7960 7960
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| slack_channel: Optional[str] = None | ||
| slack_webhook: Optional[Union[ConfigStr, str]] = None | ||
| slack_token: Optional[Union[ConfigStr, str]] = None | ||
| slack_channel: Optional[Union[ConfigStr, str]] = None |
There was a problem hiding this comment.
Should a channel be a secret str? Felt less likely than the rest but I think consistency makes sense here
There was a problem hiding this comment.
I would go ahead and keep it substituable, although not because it needs to be secret.
It adds flexibility without further complicating anything.
| slack_webhook = values["slack_webhook"] | ||
| from great_expectations.data_context.data_context.context_factory import project_manager | ||
|
|
||
| config_provider = project_manager.get_config_provider() |
There was a problem hiding this comment.
non-blocking
Just thinking out loud... but now that we can pull the config provider from the project_manager we don't actually need to pass it to ConfiStr.get_config_value(config_provider).
We could always have this method ConfiStr.get_config_value() pull from the project manager.
great_expectations/great_expectations/datasource/fluent/config_str.py
Lines 47 to 53 in ef46703
I would do this separately from this PR.
|
Hello @cdkini I've tested your implementation, and it seems to be working fine. However, the checkpoint json files are being saved with the values substituted, not the placeholders. As this file is intended to be committed alongside the other gx config files, I believe this is not an intended behavior. I think that the expected behavior should just be like the |
| values[credential] = cls._substitute_slack_credential( | ||
| slack_credential=values[credential], | ||
| config_provider=config_provider, | ||
| ) |
There was a problem hiding this comment.
@cdkini sorry I missed this in the review.
The values shouldn't be updated in the validator method. They should only be substituted when they are actually needed which looks like that would be inside the .run() method.
We should allow for
${VAR}syntax with this actioninvoke lint(usesruff format+ruff check)For more information about contributing, see Contribute.
After you submit your PR, keep the page open and monitor the statuses of the various checks made by our continuous integration process at the bottom of the page. Please fix any issues that come up and reach out on Slack if you need help. Thanks for contributing!