-
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
Add combined remote resolvers binary #5391
Add combined remote resolvers binary #5391
Conversation
/assign @vdemeester @lbernick @jerop @dibyom |
The following is the coverage report on the affected files.
|
/retest |
1 similar comment
/retest |
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.
Few comments
- Any reason for not making the
bundle
resolver disabled (even though it would be enabled by default) ? I think one of the main reason for resolvers is to be able to selectively decide which resolvers are enable and which are not (https://github.com/tektoncd/community/blob/main/teps/0060-remote-resource-resolution.md) - Naive question : any reason to go for several feature-flags instead of one called
enable-resolvers: []
that would take a list of resolvers ? It might be harder to document and validate maybe, but it could be nice as well.
It's got a feature flag - it's just reusing the existing
Good question. I'd lean towards |
/test check-pr-has-kind-label |
@abayer: The specified target(s) for
The following commands are available to trigger optional jobs:
Use In response to this:
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. |
Fair enough 🙃 then I would lean to add Code LGTM 👼🏼 |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 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 |
thanks Andrew! would you mind updating the commit message to say that the hub resolver, git resolver, and bundle resolver are being added, and some detail on the feature flags being used? happy to LGTM once vincent's comment is resolved |
1140aaa
to
5b0457a
Compare
The following is the coverage report on the affected files.
|
5b0457a
to
297012f
Compare
@lbernick Done! |
The following is the coverage report on the affected files.
|
hmm @abayer I thought you fixed the cloud events flake? it's back 😢 |
/retest @lbernick It's actually a different flake in the same test. =) The previous ones were duplicate events, this is a more general events-related flake timing out waiting for all the events to be delivered. It popped up sometimes before the duplicate event fix, and I've seen it show up on |
ah ok sg-- also it looks like your commit message refers to |
Whoops, fixing - |
Part of tektoncd#4710 This adds the `hub`, `git`, and `bundles` resolvers. The individual resolvers can be enabled via the `enable-hub-resolver`, `enable-git-resolver`, and `enable-bundles-resolver` feature flags. Signed-off-by: Andrew Bayer <andrew.bayer@gmail.com>
297012f
to
b53e0d8
Compare
@lbernick Fixed, and switched to |
The following is the coverage report on the affected files.
|
/retest |
/lgtm |
Changes
Part of #4710
/kind feature
Submitter Checklist
As the author of this PR, please check off the items in this checklist:
functionality, content, code)
/kind <type>
. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tepRelease Notes