Fix wasm workflow toml fields #16206
Conversation
| WorkflowID string `toml:"-" db:"workflow_id"` // Derived. Do not modify. the CID of the workflow. | ||
| WorkflowOwner string `toml:"-" db:"workflow_owner"` // Derived. Do not modify. the owner of the workflow. | ||
| WorkflowName string `toml:"-" db:"workflow_name"` // Derived. Do not modify. the name of the workflow. | ||
| WorkflowID string `toml:"-" db:"workflow_id"` // Derived. Do not modify. the CID of the workflow. |
There was a problem hiding this comment.
hmm. i'm confused
why is there job TOML at all for wasm wfs?
i'm concerned that changing this will break yaml, which is in prod
can this field be extracted from the another part of the existing struct?
There was a problem hiding this comment.
There was a problem hiding this comment.
Hmm isn't the change here backwards compatible? For TOML Owner + Name will not be empty, so we'll fall through to lines 916-917
There was a problem hiding this comment.
Swift do uploads of their WASM workflows via a job spec; this is something we want to move away from but atm they can't add more than one workflow because the Owner + Name isn't being populated from anywhere
| @@ -912,8 +912,12 @@ func (w *WorkflowSpec) Validate(ctx context.Context) error { | |||
| return err | |||
| } | |||
|
|
|||
| w.WorkflowOwner = strings.TrimPrefix(s.Owner, "0x") // the json schema validation ensures it is a hex string with 0x prefix, but the database does not store the prefix | |||
| w.WorkflowName = s.Name | |||
| if s.Owner+s.Name != "" { | |||
There was a problem hiding this comment.
please add a comment here about the different cases and why we need the s.Owner + s.Name check.
i'm certain i won't understand in a couple weeks.
| @@ -347,4 +348,31 @@ func TestWorkflowSpec_Validate(t *testing.T) { | |||
| require.NoError(t, err) | |||
| require.NotEmpty(t, w.WorkflowID) | |||
| }) | |||
|
|
|||
| t.Run("WASM can validate from TOML", func(t *testing.T) { | |||
There was a problem hiding this comment.
ok, i see that the existing test prove backward compatibility, yes?
There was a problem hiding this comment.
The yaml-based tests should be undisturbed, assuming that all valid yaml-based workflows have at least one of owner & name fields defined, and wasm-based tests still pass.
AER Report: CI Core ran successfully ✅AER Report: Operator UI CI ran successfully ✅ |
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
Rishabh570
dismissed stale reviews from cedric-cordenier and krehermann
via
6a9509b
|
* Fix model fields to parse correctly for a wasm workflow * casing * changeset * add comment * adds tag to changeset --------- Co-authored-by: Bartek Tofel <[email protected]> Co-authored-by: rishabh570 <[email protected]>
* Fix model fields to parse correctly for a wasm workflow * casing * changeset * add comment * adds tag to changeset --------- Co-authored-by: Bartek Tofel <[email protected]> Co-authored-by: rishabh570 <[email protected]>
* Fix model fields to parse correctly for a wasm workflow * casing * changeset * add comment * adds tag to changeset --------- Co-authored-by: Bartek Tofel <[email protected]> Co-authored-by: rishabh570 <[email protected]>
Unblocks wasm workflow users from deploying multiple workflows. Current behavior is workflowName & workflowOwner are empty, causing broken chain write behavior & preventing multiple wasm workflows from being deployed to a single node.