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

Artifacts shell 2 #4649

Merged
merged 14 commits into from
Jan 8, 2024
Merged

Artifacts shell 2 #4649

merged 14 commits into from
Jan 8, 2024

Conversation

wild-endeavor
Copy link
Contributor

@wild-endeavor wild-endeavor commented Dec 28, 2023

This is a follow up PR to the first shell pr, which just brought in the non-artifact-service artifact changes. This PR continues with additional smaller changes.

IDL changes

  • Remove the /data prefix in the grpc gateway paths.
  • Remove inputs/outputs from the cloudevents (this should cause some intermittent deserialization errors when deployed cuz the proto field numbers were changed and not deprecated)
  • DeleteTrigger changed to DeactivateTrigger
  • Path for FindByWorkflowExec updated.
  • Rename artifact_keys to artifact_trackers to be less confusing in the CloudEventExecutionStart event.

Tracking tag

Before create, the artifact service should write a string under the _ua key in the form

project/domain/name@version

where the version is the version of the artifact to be saved. At execution start, these tags are converted locally into artifact IDs containing only the artifactkey and the version (any partition information is not filled in). These IDs are then emitted alongside any artifact IDs that were the result of artifact queries, in the CloudEventExecutionStart event.

Other changes

Remove an unnecessary variable in registry.go file
Remove the initialization sql pattern in stdlib
Remove errant replace in stdlib go.mod

@wild-endeavor wild-endeavor changed the base branch from artifacts to master December 28, 2023 14:58
Copy link

codecov bot commented Dec 28, 2023

Codecov Report

Attention: 23 lines in your changes are missing coverage. Please review.

Comparison is base (513c3e1) 58.10% compared to head (63b1b81) 58.14%.
Report is 1 commits behind head on master.

❗ Current head 63b1b81 differs from pull request most recent head 8ce4ac2. Consider uploading reports for the commit 8ce4ac2 to get more accurate results

Files Patch % Lines
flyteadmin/pkg/manager/impl/execution_manager.go 24.00% 19 Missing ⚠️
flyteadmin/pkg/artifacts/registry.go 0.00% 1 Missing ⚠️
...cloudevent/implementations/cloudevent_publisher.go 0.00% 1 Missing ⚠️
flytestdlib/database/db.go 0.00% 1 Missing ⚠️
flytestdlib/database/postgres.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4649      +/-   ##
==========================================
+ Coverage   58.10%   58.14%   +0.03%     
==========================================
  Files         626      626              
  Lines       53834    53780      -54     
==========================================
- Hits        31280    31270      -10     
+ Misses      20048    20002      -46     
- Partials     2506     2508       +2     
Flag Coverage Δ
unittests 58.14% <20.68%> (+0.03%) ⬆️

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.

wild-endeavor and others added 3 commits January 5, 2024 15:45
Changes for tracking artifact usage upon execution start.

#### Tracking tag
Before create, the artifact service should write a string under the `_ua` key in the form
```
project/domain/name@version
```
where the version is the version of the artifact to be saved.  At execution start, these tags are converted locally into artifact IDs containing only the artifactkey and the version (any partition information is not filled in).  These IDs are then emitted alongside any artifact IDs that were the result of artifact queries, in the `CloudEventExecutionStart` event.

The field in the idl has been renamed from `artifact_keys` to `artifact_trackers` to be less confusing.

#### Other changes
Remove an unecessary variable in registry.go file
Remove the initialization sql pattern in stdlib
Remove errant replace in stdlib go.mod


Signed-off-by: Yee Hing Tong <[email protected]>
@wild-endeavor wild-endeavor marked this pull request as ready for review January 8, 2024 17:16
@dosubot dosubot bot added the size:XXL This PR changes 1000+ lines, ignoring generated files. label Jan 8, 2024
@dosubot dosubot bot added enhancement New feature or request housekeeping Issues that help maintain flyte and keep it tech-debt free labels Jan 8, 2024
@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Jan 8, 2024
@wild-endeavor wild-endeavor merged commit 8ac697e into master Jan 8, 2024
43 checks passed
@wild-endeavor wild-endeavor deleted the artifacts-shell-2 branch January 8, 2024 18:10
katrogan pushed a commit that referenced this pull request Jan 12, 2024
This is a follow up PR to the [first shell pr](#4474), which just brought in the non-artifact-service artifact changes.  This PR continues with additional smaller changes.

* Remove the `/data` prefix in the grpc gateway paths.
Signed-off-by: Yee Hing Tong <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request housekeeping Issues that help maintain flyte and keep it tech-debt free lgtm This PR has been approved by a maintainer size:XXL This PR changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants