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

Add provinance to the registry entry #37804

Merged
merged 2 commits into from
May 9, 2024

Conversation

bnchrch
Copy link
Contributor

@bnchrch bnchrch commented May 3, 2024

What

Adds the generation time and root metadata file info to the registry entry

image.png

Spun out of #32715 as a stack

closes https://github.com/airbytehq/airbyte-internal-issues/issues/7466

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:29pm

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
@bnchrch bnchrch force-pushed the bnchrch/registry/add-provience-metadata branch from 64a89fa to 9d7c6c2 Compare May 3, 2024 20:57
Copy link
Contributor

@natikgadzhi natikgadzhi left a comment

Choose a reason for hiding this comment

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

Haven't tested, but the code looks great. Thank you for extensive comments, and for poetry locking so we get a few vulns fixed while we're at it!


commit_sha: Optional[str] = Field(
None,
description="The git commit sha of the last commit that modified this file. DO NOT DEFINE THIS FIELD IN YOUR SPEC. It will be overwritten by the CI.",
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: should we lift the autogeneration notice up one/two levels and just say that the whole source_file_info probably should not be defined in a spec?



def default_none_to_dict(value, key, obj):
"""Set the value of a key in a dictionary to an empty dictionary if the value is None.
Copy link
Contributor

Choose a reason for hiding this comment

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

TY for a nice doc! <3

@@ -29,7 +29,7 @@ def send_slack_message(context: OpExecutionContext, channel: str, message: str,
channel (str): The channel to send the message to.
message (str): The message to send.
"""
if os.getenv("SLACK_TOKEN"):
if os.getenv("SLACK_TOKEN") == "" and os.getenv("SLACK_NOTIFICATIONS_DISABLED") != "true":
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the new env var documented somewhere?

@octavia-squidington-iv octavia-squidington-iv requested a review from a team May 5, 2024 14:19
Comment on lines +101 to +92
metadata_etag: Optional[str] = None
metadata_file_path: Optional[str] = None
metadata_bucket_name: Optional[str] = None
metadata_last_modified: Optional[str] = None
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice addition for debugging


class SourceFileInfo(BaseModel):
metadata_etag: Optional[str] = None
metadata_file_path: Optional[str] = None
Copy link
Contributor

Choose a reason for hiding this comment

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

It's a blob/key path isn't?

@@ -87,6 +117,11 @@ class BreakingChangeScope(BaseModel):
__root__: StreamBreakingChangeScope = Field(..., description="A scope that can be used to limit the impact of a breaking change.")


class GeneratedFields(BaseModel):
Copy link
Contributor

Choose a reason for hiding this comment

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

As I said in #37802 I think all generated fields should stay in the registry, not be metadata fields (which is what you're doing here 👍 )

Comment on lines 86 to 101
commit_sha: Optional[str] = Field(
None,
description="The git commit sha of the last commit that modified this file. DO NOT DEFINE THIS FIELD IN YOUR SPEC. It will be overwritten by the CI.",
)
commit_timestamp: Optional[datetime] = Field(
None,
description="The git commit timestamp of the last commit that modified this file. DO NOT DEFINE THIS FIELD IN YOUR SPEC. It will be overwritten by the CI.",
)
commit_author: Optional[str] = Field(
None,
description="The git commit author of the last commit that modified this file. DO NOT DEFINE THIS FIELD IN YOUR SPEC. It will be overwritten by the CI.",
)
commit_author_email: Optional[str] = Field(
None,
description="The git commit author email of the last commit that modified this file. DO NOT DEFINE THIS FIELD IN YOUR SPEC. It will be overwritten by the CI.",
)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is already declared in #37802
You might need to gt restack

@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-provience-metadata branch from 9d7c6c2 to 2c8909d Compare May 7, 2024 19:22
@bnchrch bnchrch force-pushed the bnchrch/registry/add-slack-tag-notifiation branch from d5ce17d to 78f8640 Compare May 8, 2024 21:34
@bnchrch bnchrch force-pushed the bnchrch/registry/add-provience-metadata branch from 2c8909d to 654f28d 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:29 AM EDT: Graphite rebased this pull request as part of a merge.
  • May 9, 10:45 AM EDT: @bnchrch merged this pull request with Graphite.

@bnchrch bnchrch force-pushed the bnchrch/registry/add-slack-tag-notifiation branch from 78f8640 to 9c54656 Compare May 9, 2024 14:19
Base automatically changed from bnchrch/registry/add-slack-tag-notifiation to master May 9, 2024 14:28
@bnchrch bnchrch force-pushed the bnchrch/registry/add-provience-metadata branch from 654f28d to 926ee78 Compare May 9, 2024 14:29
@bnchrch bnchrch merged commit f029414 into master May 9, 2024
32 checks passed
@bnchrch bnchrch deleted the bnchrch/registry/add-provience-metadata branch May 9, 2024 14:45
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