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

[DOCS] remove unsupported actions #10624

Merged
merged 15 commits into from
Nov 5, 2024
Merged

Conversation

klavavej
Copy link
Contributor

@klavavej klavavej commented Nov 4, 2024

resolves https://greatexpectations.atlassian.net/browse/DOC-844 - removes unsupported actions from the API docs (opsgenie, pagerduty, SNS)

I'm leaving UpdateDataDocsAction and ValidationAction in place as I assume those are supported and the ticket was about third party integrations that aren’t currently supported. Let me know if I’m wrong though

  • Description of PR changes above includes a link to an existing GitHub issue
  • PR title is prefixed with one of: [BUGFIX], [FEATURE], [DOCS], [MAINTENANCE], [CONTRIB]
  • Code is linted - run invoke lint (uses ruff format + ruff check)
  • Appropriate tests and docs have been updated

For more information about contributing, visit our community resources.

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!

Copy link

netlify bot commented Nov 4, 2024

Deploy Preview for niobium-lead-7998 canceled.

Name Link
🔨 Latest commit e6d4a8c
🔍 Latest deploy log https://app.netlify.com/sites/niobium-lead-7998/deploys/672952e26e6538000840acd5

Copy link

codecov bot commented Nov 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.31%. Comparing base (099f09d) to head (e6d4a8c).
Report is 1 commits behind head on develop.

✅ All tests successful. No failed tests found.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop   #10624      +/-   ##
===========================================
- Coverage    80.31%   80.31%   -0.01%     
===========================================
  Files          463      463              
  Lines        40120    40117       -3     
===========================================
- Hits         32223    32220       -3     
  Misses        7897     7897              
Flag Coverage Δ
3.10 68.01% <ø> (-0.02%) ⬇️
3.10 athena or openpyxl or pyarrow or project or sqlite or aws_creds ?
3.10 aws_deps ?
3.10 big ?
3.10 clickhouse ?
3.10 filesystem ?
3.10 mssql ?
3.10 mysql ?
3.10 postgresql ?
3.10 spark_connect ?
3.10 trino ?
3.11 68.03% <ø> (-0.01%) ⬇️
3.11 athena or openpyxl or pyarrow or project or sqlite or aws_creds ?
3.11 aws_deps ?
3.11 big ?
3.11 clickhouse ?
3.11 filesystem ?
3.11 mssql ?
3.11 mysql ?
3.11 postgresql ?
3.11 spark_connect ?
3.11 trino ?
3.12 68.03% <ø> (-0.01%) ⬇️
3.12 athena or openpyxl or pyarrow or project or sqlite or aws_creds 55.41% <ø> (-0.01%) ⬇️
3.12 aws_deps 46.14% <ø> (-0.01%) ⬇️
3.12 big 54.75% <ø> (-0.01%) ⬇️
3.12 databricks 47.88% <ø> (-0.01%) ⬇️
3.12 filesystem 61.71% <ø> (-0.01%) ⬇️
3.12 mssql 50.25% <ø> (-0.01%) ⬇️
3.12 mysql 50.31% <ø> (-0.01%) ⬇️
3.12 postgresql 54.63% <ø> (-0.01%) ⬇️
3.12 snowflake 48.85% <ø> (-0.01%) ⬇️
3.12 spark 58.06% <ø> (-0.01%) ⬇️
3.12 spark_connect 46.44% <ø> (-0.01%) ⬇️
3.12 trino 52.68% <ø> (-0.01%) ⬇️
3.9 68.06% <ø> (-0.01%) ⬇️
3.9 athena or openpyxl or pyarrow or project or sqlite or aws_creds 55.41% <ø> (-0.01%) ⬇️
3.9 aws_deps 46.17% <ø> (-0.01%) ⬇️
3.9 big 54.76% <ø> (-0.01%) ⬇️
3.9 clickhouse 43.03% <ø> (-0.01%) ⬇️
3.9 databricks 47.89% <ø> (-0.01%) ⬇️
3.9 filesystem 61.72% <ø> (-0.01%) ⬇️
3.9 mssql 50.23% <ø> (-0.01%) ⬇️
3.9 mysql 50.30% <ø> (-0.01%) ⬇️
3.9 postgresql 54.61% <ø> (-0.01%) ⬇️
3.9 snowflake 48.87% <ø> (-0.01%) ⬇️
3.9 spark 58.02% <ø> (-0.01%) ⬇️
3.9 spark_connect 46.45% <ø> (-0.01%) ⬇️
3.9 trino 52.66% <ø> (-0.01%) ⬇️
cloud 0.00% <ø> (ø)
docs-basic 53.36% <ø> (-0.01%) ⬇️
docs-creds-needed 52.93% <ø> (-0.01%) ⬇️
docs-spark 52.41% <ø> (-0.01%) ⬇️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@klavavej klavavej marked this pull request as ready for review November 4, 2024 17:39
@klavavej klavavej changed the title remove unsupported actions [DOCS] remove unsupported actions Nov 4, 2024
@@ -352,7 +352,6 @@ def _send_slack_notification(self, payload: dict) -> dict:
return {"slack_notification_result": slack_notif_result}


@public_api
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these actions no longer GX support or no longer community supported either?

That is, do we want community supported actions in our public docs and labeled as such or not appearing at all.

If these aren't community supported, can we delete the actions altogether?

Copy link
Contributor Author

@klavavej klavavej Nov 4, 2024

Choose a reason for hiding this comment

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

Are these actions no longer GX support or no longer community supported either?

These actions are unsupported by GX and unsupported by the community (source of truth for integration support is https://docs.greatexpectations.io/docs/application_integration_support#integrations)

If these aren't community supported, can we delete the actions altogether?

Yes, but that's outside the scope of this docs PR. I filed https://greatexpectations.atlassian.net/browse/CORE-600 for this and product/eng can then prioritize as y'all see fit (I'm not sure how impactful the tech debt is / if it's worth your time to clean it up or easier to just leave it in place in case product decides we should support these integrations in the future)

Copy link
Contributor

Choose a reason for hiding this comment

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

We should always remove dead code.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thinking about this more, maybe we need to leave the code in for semantic versioning. Removing the public api decorator will necessitate a minor version bump.

Copy link
Contributor

@billdirks billdirks left a comment

Choose a reason for hiding this comment

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

Thanks for working towards removing unsupported docs. It looks like the updated test no longer tests what the test name claims it does.

@@ -73,7 +73,7 @@ http://legacy.016.docs.greatexpectations.io/* http://docs.greatexpectations.io 3
/en/latest/guides/how_to_guides/miscellaneous/how_to_template.html https://legacy.docs.greatexpectations.io/en/latest/guides/how_to_guides/miscellaneous/how_to_template.html
/en/latest/guides/how_to_guides/miscellaneous/how_to_add_and_test_a_new_sqlalchemydataset_class.html https://legacy.docs.greatexpectations.io/en/latest/guides/how_to_guides/miscellaneous/how_to_add_and_test_a_new_sqlalchemydataset_class.html
/en/latest/guides/how_to_guides/miscellaneous/how_to_use_official_docker_images.html https://legacy.docs.greatexpectations.io/en/latest/guides/how_to_guides/miscellaneous/how_to_use_official_docker_images.html
/en/latest/guides/how_to_guides/miscellaneous/how_to_setup_opsgenie_alert_notifications.html https://legacy.docs.greatexpectations.io/en/latest/guides/how_to_guides/miscellaneous/how_to_setup_opsgenie_alert_notifications.html
/en/latest/guides/how_to_guides/miscellaneous/how_to_setup_opsgenie_alert_notifications.html /docs/0.18/reference/api/checkpoint/opsgeniealertaction_class/
Copy link
Contributor

Choose a reason for hiding this comment

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

Why aren't we completely removing opgenie here if it is not supported? Is it because the code still exists? When we remove that code, will we have to remove this code and update this file again?

Copy link
Contributor

Choose a reason for hiding this comment

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

That is, if we need to remove it later, could you add this to the ticket you filed, CORE-600?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

data_docs_action = UpdateDataDocsAction(name="my_docs_action")
actions: List[CheckpointAction] = [pd_action, og_action, data_docs_action]
actions: List[CheckpointAction] = [data_docs_action]
Copy link
Contributor

Choose a reason for hiding this comment

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

This test is suppose to verify sorting actions. If there is only 1 action, this test is no longer verifying this. Could you add actions to replace the ones you removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll try! But I'm definitely out of my comfort zone here so if the update I make doesn't do the trick I'd love to pair on this

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for doing this! Happy to pair.

@@ -827,4 +827,19 @@
name="get_batch_parameters_keys",
filepath=pathlib.Path("great_expectations/datasource/fluent/sql_datasource.py"),
),
IncludeExcludeDefinition(
Copy link
Contributor

Choose a reason for hiding this comment

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

If we were to remove the unsupported actions would we have to undo this change and remove these?

Copy link
Contributor

Choose a reason for hiding this comment

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

If so, could you add it as a task to do in CORE-600?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we were to remove the unsupported actions would we have to undo this change and remove these?

I assume yes, but I'm not 100% confident in my understanding of how the api docs get built

If so, could you add it as a task to do in CORE-600?

yes, will do!

Copy link
Contributor

Choose a reason for hiding this comment

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

@klavavej the api docs build is complex. I'm happy to chat about it.

Copy link
Contributor

@billdirks billdirks left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@klavavej klavavej added this pull request to the merge queue Nov 4, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 4, 2024
@klavavej klavavej added this pull request to the merge queue Nov 5, 2024
Merged via the queue into develop with commit cd1cbe4 Nov 5, 2024
70 checks passed
@klavavej klavavej deleted the kml/DOC-844/removeActions2 branch November 5, 2024 15:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants