-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
TEP-0154: Enable concise resolver syntax - stage 1 #7845
Conversation
ecfa777
to
99b1c6f
Compare
The following is the coverage report on the affected files.
|
The following is the coverage report on the affected files.
|
4ef52f1
to
b191411
Compare
b191411
to
b7bd195
Compare
The following is the coverage report on the affected files.
|
The following is the coverage report on the affected files.
|
The following is the coverage report on the affected files.
|
b7bd195
to
72a29a6
Compare
The following is the coverage report on the affected files.
|
The following is the coverage report on the affected files.
|
72a29a6
to
4d90033
Compare
The following is the coverage report on the affected files.
|
The following is the coverage report on the affected files.
|
4d90033
to
88b0beb
Compare
The following is the coverage report on the affected files.
|
The following is the coverage report on the affected files.
|
/hold cancel |
/hold |
25ae3f6
to
c068831
Compare
/hold cancel Since the required refactoring has been completed. |
The following is the coverage report on the affected files.
|
The following is the coverage report on the affected files.
|
c068831
to
d36521c
Compare
The following is the coverage report on the affected files.
|
The following is the coverage report on the affected files.
|
return errs | ||
} | ||
|
||
func validateRef(ctx context.Context, refName string, refResolver ResolverName, refParams Params) (errs *apis.FieldError) { |
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.
nit: validateRef
looks more likely to be in taskref_validation
than container_validation
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.
This is for validating the StepAction Ref. I think that's in container validation.
There is a separate validateTaskRef
which is in taskref validation.
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 am not sure I understand - ain't this change here using validateRef
as a function that's used commonly at container_validation, taskref_validation and pipeline_validation?
And maybe as you pointed explicitly name it as validateStepActionRef
might untangle this? WDYT
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.
It's tied to validating the ref. The field name for StepAction Ref is ref
. That's why I went with validateRef
since it is validating the ref
field.
d36521c
to
70ae22f
Compare
This PR enables concise resolver syntax interface.
70ae22f
to
e0fec18
Compare
The following is the coverage report on the affected files.
|
The following is the coverage report on the affected files.
|
The following is the coverage report on the affected files.
|
return bundle.ValidateParams(ctx, req.Params) | ||
} | ||
// Remove this error once validate url has been implemented. | ||
return errors.New("cannot validate request. the Validate method has not been implemented.") |
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.
Could we add test cases for those failure usage?
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.
Sure.
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.
[Not a blocker to this PR] can we have some examples to how the new syntax look like?
That will be different for each resolver since they decide the scheme. In followup PRs when we update the individual resolvers, we will add this. |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: JeromeJu, vdemeester, Yongxuanzhang The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
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
offline discussion with @chitrangpatel that we'll have more example test cases coming up in the follow-up PR with the actual scheType
s
This PR enables concise resolver syntax as per TEP 0154. It allows users to pass a url like string to
Taskref/name
field.This PR does not add the schemes supported by the
built-in
resolvers. That will come in followup PRs.Here, we just set up the framework.
Changes
enable-concise-resolver-syntax
name
such that it can also beurl-like
. Bothname
andparams
cannot be passed at the same time as discussed in the TEP. If using aurl-like
name then the feature flag must be enabled.url
which is passed in thespec
withparams
.name
like we allow in resolver params.ValidatingUrl
.ValidatingUrl
to all built-in resolvers. Currently it does not validate anything. The details will be added in a followup PR.how-to-write-a-resolver
docs.Submitter Checklist
As the author of this PR, please check off the items in this checklist:
/kind <type>
. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tepRelease Notes
/kind feature
/hold wait for TEP to be merged.