Skip to content
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 configuration types for built-in resolvers in ResolverRef #5549

Closed
abayer opened this issue Sep 23, 2022 · 10 comments
Closed

Add configuration types for built-in resolvers in ResolverRef #5549

abayer opened this issue Sep 23, 2022 · 10 comments
Assignees
Labels
kind/feature Categorizes issue or PR as related to a new feature. lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed.

Comments

@abayer
Copy link
Contributor

abayer commented Sep 23, 2022

I keep thinking that it may make sense to eventually add real fields (with relevant structs) to ResolverRef for the built-in resolvers, so that you can just do

pipelineRef:
  git:
    org: foo
    repo: bar
    revision: main
    pathInRepo: a/b/c.yaml

Rather than the more elaborate generic params approach of

pipelineRef:
  resolver: git
  params:
  - name: org
    value: foo
  - name: repo
     value: bar
  - name: revision
     value: main
  - name: pathInRepo
    value: a/b/c.yaml

We could only support the git: ... syntax for built-in resolvers, but for those, we'd be able to do admission control-time validation, a simpler syntax, PipelineRun-wide defaults, etc.

So ResolverRef would change from:

type ResolverRef struct {
	Resolver ResolverName `json:"resolver,omitempty"`
	Params []Param `json:"params,omitempty"`
}

to:

type ResolverRef struct {
	Resolver ResolverName `json:"resolver,omitempty"`
	Params []Param `json:"params,omitempty"`

	Git     *GitResolverConfig     `json:"git,omitempty"`
	Bundle  *BundlesResolverConfig `json:"bundle,omitempty"`
	Cluster *ClusterResolverConfig `json:"cluster,omitempty"`
	Hub     *HubResolverConfig     `json:"hub,omitempty"`
}        

With these new structs:

type GitResolverConfig struct {
	Org        string `json:"org,omitempty"`
	Repo       string `json:"repo,omitempty"`
	URL        string `json:"url,omitempty"`
	PathInRepo string `json:"pathInRepo,omitempty"`
	Revision   string `json:"revision,omitempty"`
}

type BundlesResolverConfig struct {
	ServiceAccount string `json:"serviceAccount,omitempty"`
	Bundle         string `json:"bundle,omitempty"`
	Name           string `json:"name,omitempty"`
	Kind           string `json:"kind,omitempty"`
}

type ClusterResolverConfig struct {
	Name      string `json:"name,omitempty"`
	Kind      string `json:"kind,omitempty"`
	Namespace string `json:"namespace,omitempty"`
}

type HubResolverConfig struct {
	Name    string `json:"name,omitempty"`
	Kind    string `json:"kind,omitempty"`
	Version string `json:"version,omitempty"`
	Catalog string `json:"catalog,omitempty"`
}

The current resolver: git + params: ... would still work for the built-in resolvers, and would be how third-party resolvers need to be used. At runtime, GetPipelineFunc and GetTaskFunc would know about these new config types, and would be able to produce a ResolutionRequest for a ref using one of them that would be identical to a ResolutionRequest created for a ref using the full params syntax.

(oh, and we get the pipelineRef.git in this scenario because PipelineRef and TaskRef both inline ResolverRef)

/kind feature

@tekton-robot tekton-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Sep 23, 2022
@abayer
Copy link
Contributor Author

abayer commented Sep 23, 2022

cc @tektoncd/core-maintainers @chuangw6 - thoughts?

@abayer
Copy link
Contributor Author

abayer commented Sep 23, 2022

/assign

@vdemeester
Copy link
Member

I am not sure I am too found of this approach, as I don't want for us to treat built-in resolvers any different from non built-in ones.
I think we should rather spend more time trying to find a way so that we could have "structured" syntax (aka no name+value param) for any type.

@abayer
Copy link
Contributor Author

abayer commented Sep 23, 2022

Also, since this idea came out of discussion prompted by #4710 (comment), we could have a new field on PipelineRun.Spec called resolverDefaults or somesuch, with this struct:

type PipelineRunResolverDefaults struct {
	Git     *GitResolverConfig     `json:"git,omitempty"`
	Bundle  *BundlesResolverConfig `json:"bundle,omitempty"`
	Cluster *ClusterResolverConfig `json:"cluster,omitempty"`
	Hub     *HubResolverConfig     `json:"hub,omitempty"`
}

@tekton-robot
Copy link
Collaborator

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale with a justification.
Stale issues rot after an additional 30d of inactivity and eventually close.
If this issue is safe to close now please do so with /close with a justification.
If this issue should be exempted, mark the issue as frozen with /lifecycle frozen with a justification.

/lifecycle stale

Send feedback to tektoncd/plumbing.

@tekton-robot tekton-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Dec 22, 2022
@tekton-robot
Copy link
Collaborator

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten with a justification.
Rotten issues close after an additional 30d of inactivity.
If this issue is safe to close now please do so with /close with a justification.
If this issue should be exempted, mark the issue as frozen with /lifecycle frozen with a justification.

/lifecycle rotten

Send feedback to tektoncd/plumbing.

@tekton-robot tekton-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Jan 21, 2023
@pritidesai
Copy link
Member

@abayer any updated on this issue? Its been rotten, do you want to bring it back out of rotten? Thanks!

@dibyom
Copy link
Member

dibyom commented Jan 24, 2023

I think we should rather spend more time trying to find a way so that we could have "structured" syntax (aka no name+value param) for any type.

Trigger interceptors allow the param value to be structured objects .

Though I now wish instead of param name/values, we just supported a single config field whose type could be structured JSON i.e. similar to this example but instead of a single param called config, we'd have have a field called config which takes a JSON that is customizable for each resolver.
Pipeline would just pass that on to each resolver which could then unmarshal it to its own config struct.

(This would address the syntax complexity issue, but not admission time validation)

@tekton-robot
Copy link
Collaborator

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen with a justification.
Mark the issue as fresh with /remove-lifecycle rotten with a justification.
If this issue should be exempted, mark the issue as frozen with /lifecycle frozen with a justification.

/close

Send feedback to tektoncd/plumbing.

@tekton-robot
Copy link
Collaborator

@tekton-robot: Closing this issue.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen with a justification.
Mark the issue as fresh with /remove-lifecycle rotten with a justification.
If this issue should be exempted, mark the issue as frozen with /lifecycle frozen with a justification.

/close

Send feedback to tektoncd/plumbing.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature Categorizes issue or PR as related to a new feature. lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed.
Projects
Status: Done
Development

No branches or pull requests

5 participants