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

Notify commit owner via slack message #37803

Merged
merged 1 commit into from
May 9, 2024

Conversation

bnchrch
Copy link
Contributor

@bnchrch bnchrch commented May 3, 2024

What

Updates our slack lifecycle notifications to mention the author of the metadata change on slack

image.png

Spun out of #32715 as a stack

Copy link

vercel bot commented May 3, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
airbyte-docs ⬜️ Ignored (Inspect) Visit Preview May 9, 2024 2:19pm

Copy link
Contributor Author

bnchrch commented May 3, 2024

@bnchrch bnchrch marked this pull request as ready for review May 3, 2024 18:43
@bnchrch bnchrch requested review from a team and alafanechere May 3, 2024 18:43
Copy link
Contributor

@alafanechere alafanechere left a comment

Choose a reason for hiding this comment

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

Minor comments, don't we need to bump the orchestrator version in pyproject.toml?

Comment on lines +435 to +436
user_identifier=user_identifier,
commit_sha=commit_sha,
Copy link
Contributor

Choose a reason for hiding this comment

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

The type hints on the log method are not accurate, they should be str | None instead of str for these two parameters.

"""
Generate the registry entry files from the given metadata file, and persist it to GCS.
"""
if not metadata_entry:
# if the metadata entry is invalid, return an empty dict
return Output(metadata={"empty_metadata": True}, value=None)

user_identifier = safe_get_slack_user_identifier(airbyte_slack_users, metadata_entry.metadata_definition)
Copy link
Contributor

Choose a reason for hiding this comment

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

(I'm not 💯 sure user_identifier makes it way to the registry, might be just used to log, feel free to ignore my comment then)

We'll publish contributors email (both airbyters and external contributors) to our registry, shall we be concerned about exposing these? Git info are public but not sure that as a contributor I'd expect my email to be publicaly available in our online registry.

@octavia-squidington-iv octavia-squidington-iv requested a review from a team May 6, 2024 09:24
@bnchrch bnchrch force-pushed the bnchrch/registry/add-git-commit-info branch from 7e76b6c to 219e7b5 Compare May 7, 2024 19:21
@bnchrch bnchrch force-pushed the bnchrch/registry/add-slack-tag-notifiation branch from 2b03faf to d5ce17d Compare May 7, 2024 19:21
@bnchrch bnchrch force-pushed the bnchrch/registry/add-git-commit-info branch from 219e7b5 to 5f9a6b9 Compare May 8, 2024 21:33
@bnchrch bnchrch force-pushed the bnchrch/registry/add-slack-tag-notifiation branch from d5ce17d to 78f8640 Compare May 8, 2024 21:34
Copy link
Contributor Author

bnchrch commented May 9, 2024

Merge activity

  • May 9, 10:18 AM EDT: @bnchrch started a stack merge that includes this pull request via Graphite.
  • May 9, 10:19 AM EDT: Graphite rebased this pull request as part of a merge.
  • May 9, 10:28 AM EDT: @bnchrch merged this pull request with Graphite.

Base automatically changed from bnchrch/registry/add-git-commit-info to master May 9, 2024 14:18
@bnchrch bnchrch force-pushed the bnchrch/registry/add-slack-tag-notifiation branch from 78f8640 to 9c54656 Compare May 9, 2024 14:19
@bnchrch bnchrch merged commit 4779b1e into master May 9, 2024
31 checks passed
@bnchrch bnchrch deleted the bnchrch/registry/add-slack-tag-notifiation branch May 9, 2024 14:28
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.

2 participants