Skip to content
This repository has been archived by the owner on Oct 9, 2023. It is now read-only.

Strip Type Metadata #442

Merged
merged 7 commits into from
May 24, 2022
Merged

Strip Type Metadata #442

merged 7 commits into from
May 24, 2022

Conversation

EngHabu
Copy link
Contributor

@EngHabu EngHabu commented May 20, 2022

Signed-off-by: Haytham Abuelfutuh [email protected]

TL;DR

  • Remove Metadata not needed at runtime from Types/Variables (structure, annotations, description). This reduces the CRD size without compromising correctness.
  • Also added flytesnacks core examples there to be compiled to help with regression testing.

Type

  • Bug Fix
  • Feature
  • Plugin

Are all requirements met?

  • Code completed
  • Smoke tested
  • Unit tests added
  • Code documentation added
  • Any pending items have an associated Issue

Tracking Issue

fixes flyteorg/flyte#2516

EngHabu added 4 commits May 19, 2022 17:04
Signed-off-by: Haytham Abuelfutuh <[email protected]>
Signed-off-by: Haytham Abuelfutuh <[email protected]>
Signed-off-by: Haytham Abuelfutuh <[email protected]>
@codecov
Copy link

codecov bot commented May 20, 2022

Codecov Report

Merging #442 (bb2f454) into master (24e93cd) will increase coverage by 0.13%.
The diff coverage is n/a.

❗ Current head bb2f454 differs from pull request most recent head e35e05f. Consider uploading reports for the commit e35e05f to get more accurate results

EngHabu added 2 commits May 19, 2022 18:40
Signed-off-by: Haytham Abuelfutuh <[email protected]>
@EngHabu EngHabu marked this pull request as ready for review May 20, 2022 01:47
@EngHabu EngHabu requested review from kumare3 and hamersaw as code owners May 20, 2022 01:47
@kumare3
Copy link
Contributor

kumare3 commented May 20, 2022

Ohh man do we want to check in all examples?

@EngHabu
Copy link
Contributor Author

EngHabu commented May 20, 2022

Any issue with that? Regression testing?
I realized E2E tests do not test compiler changes because they require updating and rebuilding admin... until they do, I would like some validation beforehand...

hamersaw
hamersaw previously approved these changes May 23, 2022
Copy link
Contributor

@hamersaw hamersaw left a comment

Choose a reason for hiding this comment

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

If I understand this correctly, can we remove this section in the buildTasks function which strips annotations from the LiteralType since this PR removes them as part of the StripTypeMetadata function?

Once that is figured out, looks great! I wonder how much space this saves on the CRD.

Signed-off-by: Haytham Abuelfutuh <[email protected]>
@EngHabu
Copy link
Contributor Author

EngHabu commented May 24, 2022

Excellent point @hamersaw

@EngHabu EngHabu requested a review from hamersaw May 24, 2022 00:13
@EngHabu EngHabu merged commit 755e90a into master May 24, 2022
@EngHabu EngHabu deleted the strip-type-metadata branch May 24, 2022 03:06
eapolinario pushed a commit to eapolinario/flytepropeller that referenced this pull request Aug 9, 2023
* Strip Type Metadata

Signed-off-by: Haytham Abuelfutuh <[email protected]>

* Strip variable descriptions

Signed-off-by: Haytham Abuelfutuh <[email protected]>

* Strip Description

Signed-off-by: Haytham Abuelfutuh <[email protected]>

* Use a fixed time for datetime field literals

Signed-off-by: Haytham Abuelfutuh <[email protected]>

* Move logic to strip metadata to k8s transformer

Signed-off-by: Haytham Abuelfutuh <[email protected]>

* Update code documentation

Signed-off-by: Haytham Abuelfutuh <[email protected]>

* PR Comments

Signed-off-by: Haytham Abuelfutuh <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Core feature] Server-side compiler should strip Type Metadata
3 participants