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

CRD validation on flytepropeller project structure #328

Merged
merged 9 commits into from
Oct 28, 2021
Merged

CRD validation on flytepropeller project structure #328

merged 9 commits into from
Oct 28, 2021

Conversation

pingsutw
Copy link
Member

@pingsutw pingsutw commented Oct 6, 2021

TL;DR

Adding CRD Validation for Flyteworkflw

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

Complete description

auto-generate CRD YAML with openapi schema

Tracking Issue

flyteorg/flyte#355

Follow-up issue

NA

@pingsutw pingsutw force-pushed the CRD-validation branch 4 times, most recently from f91ce39 to 772c107 Compare October 7, 2021 12:01
@codecov
Copy link

codecov bot commented Oct 7, 2021 •
edited
Loading

Codecov Report

Merging #328 (6a2ad1b) into master (5f37d78) will decrease coverage by 6.29%.
The diff coverage is 11.88%.

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

EngHabu
EngHabu reviewed Oct 8, 2021
View reviewed changes
Copy link
Contributor

@EngHabu EngHabu left a comment

Choose a reason for hiding this comment

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

I think this is great! I've some comments on the serialization fields... generally speaking, we shouldn't change the current structure, otherwise, deserializing existing workflow objects will fail..

pkg/apis/flyteworkflow/v1alpha1/branch.go Show resolved Hide resolved
pkg/apis/flyteworkflow/v1alpha1/branch.go Outdated
@@ -37,7 +37,7 @@ func (in *Error) DeepCopyInto(out *Error) {
}

type BooleanExpression struct {
*core.BooleanExpression
*core.BooleanExpression `json:"-"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not serialize this? that doesn't look right

Copy link
Member Author

@pingsutw pingsutw Oct 13, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

It will cause an error from controller-gen if we use json:",inline".
There is no JSON tag in type "BooleanExpression", and that struct is generated from Flyteidl.

type BooleanExpression struct {
	// Types that are valid to be assigned to Expr:
	//	*BooleanExpression_Conjunction
	//	*BooleanExpression_Comparison
	Expr                 isBooleanExpression_Expr `protobuf_oneof:"expr"`
	XXX_NoUnkeyedLiteral struct{}                 `json:"-"`
	XXX_unrecognized     []byte                   `json:"-"`
	XXX_sizecache        int32                    `json:"-"`
}

Error message:

/Users/kevin/go/src/github.com/flyteorg/flytepropeller/pkg/apis/flyteworkflow/v1alpha1/branch.go:40:2: 
encountered struct field "" without JSON tag in type "BooleanExpression"

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed json:"-" here.
It seems like controller-gen can still generate a CRD file although there are some errors.
Can we just Ignore those errors?

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 what that means.... can you run a workflow in sandbox and see what gets serialized in the Workflow CRD, then apply your changes and rerun the workflow... and capture the CRD... the expectation is there there should be no diff... might be helpful to post the diff here as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

There is no diff. https://www.textcompare.org/yaml/?id=6177b9ca537b150013e697f9.

pkg/apis/flyteworkflow/v1alpha1/execution_config.go Outdated Show resolved Hide resolved
pkg/apis/flyteworkflow/v1alpha1/nodes.go Outdated Show resolved Hide resolved
pkg/apis/flyteworkflow/v1alpha1/nodes.go Outdated Show resolved Hide resolved
pkg/apis/flyteworkflow/v1alpha1/tasks.go Outdated Show resolved Hide resolved
pkg/apis/flyteworkflow/v1alpha1/workflow.go Outdated Show resolved Hide resolved
pkg/apis/flyteworkflow/v1alpha1/workflow.go Outdated Show resolved Hide resolved
@pingsutw
CRD validation
396ad4c 
Signed-off-by: Kevin Su <[email protected]>
@pingsutw
CRD validation
4104d4d 
Signed-off-by: Kevin Su <[email protected]>
@pingsutw
Revert json tag name
6ece8fc 
Signed-off-by: Kevin Su <[email protected]>
@pingsutw
Address comment
9165b73 
Signed-off-by: Kevin Su <[email protected]>
@pingsutw
Address comment
5be88fd 
Signed-off-by: Kevin Su <[email protected]>
@pingsutw
Fixed test
323a93f 
Signed-off-by: Kevin Su <[email protected]>
@pingsutw
Rebase
b2ea25f 
Signed-off-by: Kevin Su <[email protected]>
@pingsutw
Updated CRD
69c2759 
Signed-off-by: Kevin Su <[email protected]>
@pingsutw pingsutw force-pushed the CRD-validation branch 3 times, most recently from 87f21e6 to c11107e Compare October 27, 2021 17:54
@pingsutw
test
fa992f2 
Signed-off-by: Kevin Su <[email protected]>
@pingsutw
Copy link
Member Author

Few updates:

  • Rename CRD (the name should be the same as the CRD in flyte repo)
  • Add a GitHub action workflow to make sure the CRD file is up to date.
  • Add short name

@EngHabu
Copy link
Contributor

EngHabu commented Oct 28, 2021

I applied the generated CRD and ran:

  • core.advanced.run_merge_sort.merge_sort
  • core.control_flow.run_conditions.multiplier
  • kfmpi.mpi_mnist.horovod_training_wf
  • core.scheduled_workflows.lp_schedules.date_formatter_wf

successfully

@EngHabu EngHabu merged commit 02f2ad9 into flyteorg:master Oct 28, 2021
EngHabu added a commit that referenced this pull request Oct 28, 2021
@EngHabu
Revert "CRD validation on flytepropeller project structure (#328)"
a0b82bd 
This reverts commit 02f2ad9.
@EngHabu EngHabu mentioned this pull request Oct 28, 2021
Merged
EngHabu added a commit that referenced this pull request Oct 28, 2021
@EngHabu
Revert "CRD validation on flytepropeller project structure (#328)"
1e9f29c 
This reverts commit 02f2ad9.

Signed-off-by: Haytham Abuelfutuh <[email protected]>
EngHabu added a commit that referenced this pull request Oct 28, 2021
@EngHabu
Revert "CRD validation on flytepropeller project structure (#328)" (#355
6a9c1d4 
)

This reverts commit 02f2ad9.

Signed-off-by: Haytham Abuelfutuh <[email protected]>
pingsutw
pingsutw commented Oct 28, 2021
View reviewed changes
pkg/apis/flyteworkflow/v1alpha1/node_status.go
Comment on lines -161 to -162
Phase WorkflowNodePhase `json:"phase,omitempty"`
ExecutionError *core.ExecutionError `json:"executionError,omitempty"`
Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe we should not change this.

eapolinario pushed a commit to eapolinario/flytepropeller that referenced this pull request Aug 9, 2023
@pingsutw
CRD validation on flytepropeller project structure (flyteorg#328)
0cac412 
* CRD validation

Signed-off-by: Kevin Su <[email protected]>

* CRD validation

Signed-off-by: Kevin Su <[email protected]>

* Revert json tag name

Signed-off-by: Kevin Su <[email protected]>

* Address comment

Signed-off-by: Kevin Su <[email protected]>

* Address comment

Signed-off-by: Kevin Su <[email protected]>

* Fixed test

Signed-off-by: Kevin Su <[email protected]>

* Rebase

Signed-off-by: Kevin Su <[email protected]>

* Updated CRD

Signed-off-by: Kevin Su <[email protected]>

* test

Signed-off-by: Kevin Su <[email protected]>
eapolinario pushed a commit to eapolinario/flytepropeller that referenced this pull request Aug 9, 2023
@EngHabu
Revert "CRD validation on flytepropeller project structure (flyteorg#328
3d16575 
)" (flyteorg#355)

This reverts commit 0cac412.

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

@EngHabu EngHabu EngHabu approved these changes

@kumare3 kumare3 Awaiting requested review from kumare3 kumare3 is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@pingsutw @EngHabu