-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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 table support for capability "type" property #12622
Conversation
I see you updated files related to |
5594add
to
a94d96d
Compare
a94d96d
to
114388a
Compare
core/services/workflows/testdata/fixtures/workflows/workflow_schema.json
Show resolved
Hide resolved
@@ -113,6 +113,14 @@ require ( | |||
gopkg.in/natefinch/lumberjack.v2 v2.2.1 | |||
) | |||
|
|||
require ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What changed that made a go.mod update necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
im not sure, i didnt change any deps..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah in that case, can we remove the changes to this file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i tried, but it gets added back in when i run go tidy
// stepDefinition is the parsed representation of a step in a workflow. | ||
// | ||
// Within the workflow spec, they are called "Capability Properties". | ||
type stepDefinition struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is models.go moved from workflow.go? What is confusing to me that when I look at the commits separately, none of them deletes workflow.go ... :| It's a bit hard for me to understand what changed. Can you maybe split this PR into pure file rename + actual changes separately?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, sorry about that. I made the rename be the last commit. The meaty portion of the PR is in the first commit, the refactor into two files is the second commit.
It should be much easier to individually review the commits now.
acc20e1
to
dfe5e11
Compare
Quality Gate passedIssues Measures |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM but I'll let Cedric approve
core/services/workflows/workflow.go
Outdated
tableSchema := reflector.Reflect(&stepDefinitionTableType{}) | ||
stringSchema := &jsonschema.Schema{ | ||
Type: "string", | ||
Pattern: "^[a-z0-9_\\-:]+@(0|[1-9]\\d*)(?:-((?:0|[1-9]\\d*|\\d*[a-zA-Z-][0-9a-zA-Z-]*)(?:\\.(?:0|[1-9]\\d*|\\d*[a-zA-Z-][0-9a-zA-Z-]*))*))?(?:\\+([0-9a-zA-Z-]+(?:\\.[0-9a-zA-Z-]+)*))?$", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a comment explaining this regex with an example?
…ersion * develop: (32 commits) [KS-136] Write target fixes (#12743) chore/release 2.10.0 to develop (#12740) [KS-136] Disallow non-trigger steps with no dependent ref (#12742) [KS-136] Correctly handle numbers in YAML by converting them to floats or ints (#12739) New log buffer (#12357) [KS-101] Add OCR3 capability contract wrapper (#12404) core/services/relay/evm: switch RequestRound DB & Tracker to use sqlutil.DataSource (#12706) Unregister filters for old coordinator contracts contract addresses from Functions LogPollerWrapper (#12696) Add table support for capability "type" property (#12622) Backout CRIB setup on develop. (#12705) fix node upgrade test (#12702) Reduces changeset scope to `minor` for semver (#12699) rm oz dep (#12700) @chainlink.contracts release v1.0.0 (#11714) feat: contracts publishing in CI (#12102) Bump default PG conns from 20->100; enable auto-scaling open conns for mercury (#12697) chore: chainlink-github-actions/* to v2.3.10 (#12694) LOOPP plugin config validation service (#12430) [TT-924] Migrate functions load tests to Seth (#12659) Enhance automation test config (AUTO-9430) (#12689) ...
The "type" property for workflow step definitions can now be represented as a string or table.
Example (string)
Example (table)
The internal representation
workflowSpec
still keepstype
as a string. See https://github.com/smartcontractkit/chainlink/pull/12622/files#diff-8cca50c191da32f2d035c361493a37ad6cc1f0c0d0620a5a71c810d3a5d7fcb1R218