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

TEP-0154 Concise Remote Resolver Syntax #1138

Merged
merged 1 commit into from
Apr 5, 2024

Conversation

chitrangpatel
Copy link
Contributor

This PR proposes to include a concise remote resolver syntax to reference remote resources.
It addresses the feature request tektoncd/pipeline#7508.

/kind tep

@tekton-robot tekton-robot added the kind/tep Categorizes issue or PR as related to a TEP (or needs a TEP). label Mar 12, 2024
@tekton-robot tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Mar 12, 2024
@chitrangpatel
Copy link
Contributor Author

/assign @wlynch

@chitrangpatel
Copy link
Contributor Author

cc @chmouel PTAL (specifically for http-resolver) 🙂

teps/0154-concise-remote-resolver-syntax.md Outdated Show resolved Hide resolved
teps/0154-concise-remote-resolver-syntax.md Outdated Show resolved Hide resolved
teps/0154-concise-remote-resolver-syntax.md Outdated Show resolved Hide resolved
teps/0154-concise-remote-resolver-syntax.md Outdated Show resolved Hide resolved
teps/0154-concise-remote-resolver-syntax.md Outdated Show resolved Hide resolved
@chitrangpatel chitrangpatel force-pushed the remote-resolver-syntax branch 4 times, most recently from b935354 to c0f6253 Compare March 14, 2024 14:55
@chitrangpatel chitrangpatel requested a review from wlynch March 14, 2024 15:01
Copy link
Member

@wlynch wlynch left a comment

Choose a reason for hiding this comment

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

/lgtm

IIUC we loosened the constraints enough that tl;dr this is adding url to the remote resolver request.

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 15, 2024
@dibyom
Copy link
Member

dibyom commented Mar 15, 2024

/approve
/hold

Added a hold in case other reviewers want to take a look before the TEP is merged. Feel free to remove after

@tekton-robot tekton-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Mar 15, 2024
@chitrangpatel chitrangpatel force-pushed the remote-resolver-syntax branch from c0f6253 to 9710ad5 Compare March 19, 2024 13:58
@tekton-robot tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label Mar 19, 2024
@chitrangpatel chitrangpatel requested a review from chmouel March 19, 2024 13:58
@chitrangpatel chitrangpatel force-pushed the remote-resolver-syntax branch from 9710ad5 to 7286131 Compare March 19, 2024 14:04
Copy link
Member

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

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

If we want to be consice, couldn't we use name for this instead adding url and duplicating some information (the name) ? I think it would be backward compatible : it's {name}://… we infer the name and we don't read the rest, it's {name} only it's the current syntax that applies. In ResolutionRequest we would pass the url or the parameters and let the resolver handle that (and/or the helper code we provide).

Can we write an example for a custom resolver ? They are part of the resolver "feature set" (to be able to write your own resolver) and thus we should make sure we take them into account in this proposal.

teps/0154-concise-remote-resolver-syntax.md Outdated Show resolved Hide resolved
teps/0154-concise-remote-resolver-syntax.md Outdated Show resolved Hide resolved
teps/0154-concise-remote-resolver-syntax.md Outdated Show resolved Hide resolved
teps/0154-concise-remote-resolver-syntax.md Outdated Show resolved Hide resolved
teps/0154-concise-remote-resolver-syntax.md Outdated Show resolved Hide resolved
@chitrangpatel
Copy link
Contributor Author

chitrangpatel commented Mar 19, 2024

If we want to be consice, couldn't we use name for this instead adding url and duplicating some information (the name) ? I think it would be backward compatible : it's {name}://… we infer the name and we don't read the rest, it's {name} only it's the current syntax that applies. In ResolutionRequest we would pass the url or the parameters and let the resolver handle that (and/or the helper code we provide).

Just the name alone won't help. That's because it then requires that the value starts with the resolver name. And that does not work with something like the http resolver where the scheme could be http or https.
e.g.
https://github.com/tektoncd/pipeline is probably meant to go to the git resolver.
https://raw.githubusercontent.com/tektoncd/pipeline/main/examples/v1/taskruns/array-default.yaml is meant to go to the http resolver.

So in the above case, the name alone does not work. But name with the resolver works because we know where the resolution request is supposed to go:

taskRef:
  name: "https://github.com/tektoncd/pipeline"
  resolver: git

Can we write an example for a custom resolver ? They are part of the resolver "feature set" (to be able to write your own resolver) and thus we should make sure we take them into account in this proposal.

Can do!

@vdemeester
Copy link
Member

@chitrangpatel, yeah, as you commented here, we would want "resolver" to be able to say "I handle those sheme", and some sort of priority.

Now, even with that, the git+https is a slight… tricky one to achieve. If the http resolver has a higher priority than git, would we always let the http resolver handle it, fail, and give it to the next ? In case of an 401 (unauthorized), would we still do the same (with a high level of chance it will be 401 with the git provider as well).

Some history note (that might not have any "impact"), the current syntax is also there because it was decided to be consistent with the rest of the API (on params). The initial proposal looked like

  name: 
  params:
    - foo: bar
    - {name}: {value}

@vdemeester vdemeester self-assigned this Mar 20, 2024
@chitrangpatel chitrangpatel force-pushed the remote-resolver-syntax branch from 7286131 to 3683ff3 Compare March 21, 2024 17:31
@tekton-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dibyom, vdemeester

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@chitrangpatel
Copy link
Contributor Author

/hold cancel

@tekton-robot tekton-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 5, 2024
Copy link
Member

@wlynch wlynch left a comment

Choose a reason for hiding this comment

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

/lgtm

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 5, 2024
@tekton-robot tekton-robot merged commit 703ed5e into tektoncd:main Apr 5, 2024
3 checks passed
chitrangpatel added a commit to chitrangpatel/pipeline that referenced this pull request Apr 29, 2024
This PR implements an updated resolver framework with slight updates. This is to avoid backwards incompatibility while implementing [TEP-0154](tektoncd/community#1138).

The current framework only works with Params. e.g. The interface has ValidateParams and Resolve which takes in Params. Now that we also need to pass in a `URL`, we need to add new methods and change function signatures which leads to API incompatibility with existing custom resolvers. As a result, when users upgrade to new version of Tekton Pipelines, they will be forced to be compatible with the new format because of the interface changes.

This PR tries to make it future proof such that if we add new fields to the ResolutionSpec, it will be handled without the need to break users.
chitrangpatel added a commit to chitrangpatel/pipeline that referenced this pull request May 1, 2024
This PR implements an updated resolver framework with slight updates. This is to avoid backwards incompatibility while implementing [TEP-0154](tektoncd/community#1138).

The current framework only works with Params. e.g. The interface has ValidateParams and Resolve which takes in Params. Now that we also need to pass in a `URL`, we need to add new methods and change function signatures which leads to API incompatibility with existing custom resolvers. As a result, when users upgrade to new version of Tekton Pipelines, they will be forced to be compatible with the new format because of the interface changes.

This PR tries to make it future proof such that if we add new fields to the ResolutionSpec, it will be handled without the need to break users.
chitrangpatel added a commit to chitrangpatel/pipeline that referenced this pull request May 1, 2024
This PR implements an updated resolver framework with slight updates. This is to avoid backwards incompatibility while implementing [TEP-0154](tektoncd/community#1138).

The current framework only works with Params. e.g. The interface has ValidateParams and Resolve which takes in Params. Now that we also need to pass in a `URL`, we need to add new methods and change function signatures which leads to API incompatibility with existing custom resolvers. As a result, when users upgrade to new version of Tekton Pipelines, they will be forced to be compatible with the new format because of the interface changes.

This PR tries to make it future proof such that if we add new fields to the ResolutionSpec, it will be handled without the need to break users.
chitrangpatel added a commit to chitrangpatel/pipeline that referenced this pull request May 1, 2024
This PR implements an updated resolver framework with slight updates. This is to avoid backwards incompatibility while implementing [TEP-0154](tektoncd/community#1138).

The current framework only works with Params. e.g. The interface has ValidateParams and Resolve which takes in Params. Now that we also need to pass in a `URL`, we need to add new methods and change function signatures which leads to API incompatibility with existing custom resolvers. As a result, when users upgrade to new version of Tekton Pipelines, they will be forced to be compatible with the new format because of the interface changes.

This PR tries to make it future proof such that if we add new fields to the ResolutionSpec, it will be handled without the need to break users.
chitrangpatel added a commit to chitrangpatel/pipeline that referenced this pull request May 1, 2024
This PR implements an updated resolver framework with slight updates. This is to avoid backwards incompatibility while implementing [TEP-0154](tektoncd/community#1138).

The current framework only works with Params. e.g. The interface has ValidateParams and Resolve which takes in Params. Now that we also need to pass in a `URL`, we need to add new methods and change function signatures which leads to API incompatibility with existing custom resolvers. As a result, when users upgrade to new version of Tekton Pipelines, they will be forced to be compatible with the new format because of the interface changes.

This PR tries to make it future proof such that if we add new fields to the ResolutionSpec, it will be handled without the need to break users.
chitrangpatel added a commit to chitrangpatel/pipeline that referenced this pull request May 9, 2024
This PR implements an updated resolver framework with slight updates. This is to avoid backwards incompatibility while implementing [TEP-0154](tektoncd/community#1138).

The current framework only works with Params. e.g. The interface has ValidateParams and Resolve which takes in Params. Now that we also need to pass in a `URL`, we need to add new methods and change function signatures which leads to API incompatibility with existing custom resolvers. As a result, when users upgrade to new version of Tekton Pipelines, they will be forced to be compatible with the new format because of the interface changes.

This PR tries to make it future proof such that if we add new fields to the ResolutionSpec, it will be handled without the need to break users.

more refactor
chitrangpatel added a commit to chitrangpatel/pipeline that referenced this pull request May 9, 2024
This PR implements an updated resolver framework with slight updates. This is to avoid backwards incompatibility while implementing [TEP-0154](tektoncd/community#1138).

The current framework only works with Params. e.g. The interface has ValidateParams and Resolve which takes in Params. Now that we also need to pass in a `URL`, we need to add new methods and change function signatures which leads to API incompatibility with existing custom resolvers. As a result, when users upgrade to new version of Tekton Pipelines, they will be forced to be compatible with the new format because of the interface changes.

This PR tries to make it future proof such that if we add new fields to the ResolutionSpec, it will be handled without the need to break users.
chitrangpatel added a commit to chitrangpatel/pipeline that referenced this pull request May 10, 2024
This PR implements an updated resolver framework with slight updates. This is to avoid backwards incompatibility while implementing [TEP-0154](tektoncd/community#1138).

The current framework only works with Params. e.g. The interface has ValidateParams and Resolve which takes in Params. Now that we also need to pass in a `URL`, we need to add new methods and change function signatures which leads to API incompatibility with existing custom resolvers. As a result, when users upgrade to new version of Tekton Pipelines, they will be forced to be compatible with the new format because of the interface changes.

This PR tries to make it future proof such that if we add new fields to the ResolutionSpec, it will be handled without the need to break users.
chitrangpatel added a commit to chitrangpatel/pipeline that referenced this pull request May 10, 2024
This PR implements an updated resolver framework with slight updates. This is to avoid backwards incompatibility while implementing [TEP-0154](tektoncd/community#1138).

The current framework only works with Params. e.g. The interface has ValidateParams and Resolve which takes in Params. Now that we also need to pass in a `URL`, we need to add new methods and change function signatures which leads to API incompatibility with existing custom resolvers. As a result, when users upgrade to new version of Tekton Pipelines, they will be forced to be compatible with the new format because of the interface changes.

This PR tries to make it future proof such that if we add new fields to the ResolutionSpec, it will be handled without the need to break users.
chitrangpatel added a commit to chitrangpatel/pipeline that referenced this pull request May 10, 2024
This PR implements an updated resolver framework with slight updates. This is to avoid backwards incompatibility while implementing [TEP-0154](tektoncd/community#1138).

The current framework only works with Params. e.g. The interface has ValidateParams and Resolve which takes in Params. Now that we also need to pass in a `URL`, we need to add new methods and change function signatures which leads to API incompatibility with existing custom resolvers. As a result, when users upgrade to new version of Tekton Pipelines, they will be forced to be compatible with the new format because of the interface changes.

This PR tries to make it future proof such that if we add new fields to the ResolutionSpec, it will be handled without the need to break users.
chitrangpatel added a commit to chitrangpatel/pipeline that referenced this pull request May 10, 2024
This PR implements an updated resolver framework with slight updates. This is to avoid backwards incompatibility while implementing [TEP-0154](tektoncd/community#1138).

The current framework only works with Params. e.g. The interface has ValidateParams and Resolve which takes in Params. Now that we also need to pass in a `URL`, we need to add new methods and change function signatures which leads to API incompatibility with existing custom resolvers. As a result, when users upgrade to new version of Tekton Pipelines, they will be forced to be compatible with the new format because of the interface changes.

This PR tries to make it future proof such that if we add new fields to the ResolutionSpec, it will be handled without the need to break users.
chitrangpatel added a commit to chitrangpatel/pipeline that referenced this pull request May 10, 2024
This PR implements an updated resolver framework with slight updates. This is to avoid backwards incompatibility while implementing [TEP-0154](tektoncd/community#1138).

The current framework only works with Params. e.g. The interface has ValidateParams and Resolve which takes in Params. Now that we also need to pass in a `URL`, we need to add new methods and change function signatures which leads to API incompatibility with existing custom resolvers. As a result, when users upgrade to new version of Tekton Pipelines, they will be forced to be compatible with the new format because of the interface changes.

This PR tries to make it future proof such that if we add new fields to the ResolutionSpec, it will be handled without the need to break users.
chitrangpatel added a commit to chitrangpatel/pipeline that referenced this pull request May 10, 2024
This PR implements an updated resolver framework with slight updates. This is to avoid backwards incompatibility while implementing [TEP-0154](tektoncd/community#1138).

The current framework only works with Params. e.g. The interface has ValidateParams and Resolve which takes in Params. Now that we also need to pass in a `URL`, we need to add new methods and change function signatures which leads to API incompatibility with existing custom resolvers. As a result, when users upgrade to new version of Tekton Pipelines, they will be forced to be compatible with the new format because of the interface changes.

This PR tries to make it future proof such that if we add new fields to the ResolutionSpec, it will be handled without the need to break users.
chitrangpatel added a commit to chitrangpatel/pipeline that referenced this pull request May 10, 2024
This PR implements an updated resolver framework with slight updates. This is to avoid backwards incompatibility while implementing [TEP-0154](tektoncd/community#1138).

The current framework only works with Params. e.g. The interface has ValidateParams and Resolve which takes in Params. Now that we also need to pass in a `URL`, we need to add new methods and change function signatures which leads to API incompatibility with existing custom resolvers. As a result, when users upgrade to new version of Tekton Pipelines, they will be forced to be compatible with the new format because of the interface changes.

This PR tries to make it future proof such that if we add new fields to the ResolutionSpec, it will be handled without the need to break users.
chitrangpatel added a commit to chitrangpatel/pipeline that referenced this pull request May 13, 2024
This PR implements an updated resolver framework with slight updates. This is to avoid backwards incompatibility while implementing [TEP-0154](tektoncd/community#1138).

The current framework only works with Params. e.g. The interface has ValidateParams and Resolve which takes in Params. Now that we also need to pass in a `URL`, we need to add new methods and change function signatures which leads to API incompatibility with existing custom resolvers. As a result, when users upgrade to new version of Tekton Pipelines, they will be forced to be compatible with the new format because of the interface changes.

This PR tries to make it future proof such that if we add new fields to the ResolutionSpec, it will be handled without the need to break users.
chitrangpatel added a commit to chitrangpatel/pipeline that referenced this pull request May 14, 2024
This PR implements an updated resolver framework with slight updates. This is to avoid backwards incompatibility while implementing [TEP-0154](tektoncd/community#1138).

The current framework only works with Params. e.g. The interface has ValidateParams and Resolve which takes in Params. Now that we also need to pass in a `URL`, we need to add new methods and change function signatures which leads to API incompatibility with existing custom resolvers. As a result, when users upgrade to new version of Tekton Pipelines, they will be forced to be compatible with the new format because of the interface changes.

This PR tries to make it future proof such that if we add new fields to the ResolutionSpec, it will be handled without the need to break users.
chitrangpatel added a commit to chitrangpatel/pipeline that referenced this pull request May 14, 2024
This PR implements an updated resolver framework with slight updates. This is to avoid backwards incompatibility while implementing [TEP-0154](tektoncd/community#1138).

The current framework only works with Params. e.g. The interface has ValidateParams and Resolve which takes in Params. Now that we also need to pass in a `URL`, we need to add new methods and change function signatures which leads to API incompatibility with existing custom resolvers. As a result, when users upgrade to new version of Tekton Pipelines, they will be forced to be compatible with the new format because of the interface changes.

This PR tries to make it future proof such that if we add new fields to the ResolutionSpec, it will be handled without the need to break users.
chitrangpatel added a commit to chitrangpatel/pipeline that referenced this pull request May 14, 2024
This PR implements an updated resolver framework with slight updates. This is to avoid backwards incompatibility while implementing [TEP-0154](tektoncd/community#1138).

The current framework only works with Params. e.g. The interface has ValidateParams and Resolve which takes in Params. Now that we also need to pass in a `URL`, we need to add new methods and change function signatures which leads to API incompatibility with existing custom resolvers. As a result, when users upgrade to new version of Tekton Pipelines, they will be forced to be compatible with the new format because of the interface changes.

This PR tries to make it future proof such that if we add new fields to the ResolutionSpec, it will be handled without the need to break users.
chitrangpatel added a commit to chitrangpatel/pipeline that referenced this pull request May 14, 2024
This PR implements an updated resolver framework with slight updates. This is to avoid backwards incompatibility while implementing [TEP-0154](tektoncd/community#1138).

The current framework only works with Params. e.g. The interface has ValidateParams and Resolve which takes in Params. Now that we also need to pass in a `URL`, we need to add new methods and change function signatures which leads to API incompatibility with existing custom resolvers. As a result, when users upgrade to new version of Tekton Pipelines, they will be forced to be compatible with the new format because of the interface changes.

This PR tries to make it future proof such that if we add new fields to the ResolutionSpec, it will be handled without the need to break users.
chitrangpatel added a commit to chitrangpatel/pipeline that referenced this pull request May 14, 2024
This PR implements an updated resolver framework with slight updates. This is to avoid backwards incompatibility while implementing [TEP-0154](tektoncd/community#1138).

The current framework only works with Params. e.g. The interface has ValidateParams and Resolve which takes in Params. Now that we also need to pass in a `URL`, we need to add new methods and change function signatures which leads to API incompatibility with existing custom resolvers. As a result, when users upgrade to new version of Tekton Pipelines, they will be forced to be compatible with the new format because of the interface changes.

This PR tries to make it future proof such that if we add new fields to the ResolutionSpec, it will be handled without the need to break users.
tekton-robot pushed a commit to tektoncd/pipeline that referenced this pull request May 14, 2024
This PR implements an updated resolver framework with slight updates. This is to avoid backwards incompatibility while implementing [TEP-0154](tektoncd/community#1138).

The current framework only works with Params. e.g. The interface has ValidateParams and Resolve which takes in Params. Now that we also need to pass in a `URL`, we need to add new methods and change function signatures which leads to API incompatibility with existing custom resolvers. As a result, when users upgrade to new version of Tekton Pipelines, they will be forced to be compatible with the new format because of the interface changes.

This PR tries to make it future proof such that if we add new fields to the ResolutionSpec, it will be handled without the need to break users.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/tep Categorizes issue or PR as related to a TEP (or needs a TEP). lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants