-
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
Move remote resolution out of alpha #5515
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -23,18 +23,10 @@ metadata: | |
app.kubernetes.io/part-of: tekton-pipelines | ||
data: | ||
# Setting this flag to "true" enables remote resolution of Tekton OCI bundles. | ||
# This is an experimental feature and thus should still be considered | ||
# an alpha feature. | ||
enable-bundles-resolver: "false" | ||
enable-bundles-resolver: "true" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why are we changing this as part of making this feature There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, I can do that in a separate PR, if so desired. I wasn't a big fan of having them off by default in the first place, but the more I thought about it, the more I decided that it was a mistake to have them off. I just thought the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it's OK to make this swap in the same PR, but I'll defer to @afrittoli here. Just FYI that for v1 we're hoping to have resolution in beta and these resolvers (at least the cluster and bundle ones) enabled by default, as replacements for the OCI bundles syntax and ClusterTask. (ClusterTask might be less necessary since it's a separate CRD, but either way, I think we're waiting on #5579 until this is merged.) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I understand the reasoning for switching those flags by default, and since this PR is already baked (and rebased enough times) I think it's fine to keep them together - but generally I would prefer if we avoided grouping changes in one PR. I think a more natural flow would have been switching the flag default first and moving to beta then. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 Will do in the future. |
||
# Setting this flag to "true" enables remote resolution of tasks and pipelines via the Tekton Hub. | ||
# This is an experimental feature and thus should still be considered | ||
# an alpha feature. | ||
enable-hub-resolver: "false" | ||
enable-hub-resolver: "true" | ||
# Setting this flag to "true" enables remote resolution of tasks and pipelines from Git repositories. | ||
# This is an experimental feature and thus should still be considered | ||
# an alpha feature. | ||
enable-git-resolver: "false" | ||
enable-git-resolver: "true" | ||
# Setting this flag to "true" enables remote resolution of tasks and pipelines from other namespaces within the cluster. | ||
# This is an experimental feature and thus should still be considered | ||
# an alpha feature. | ||
enable-cluster-resolver: "false" | ||
enable-cluster-resolver: "true" |
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 guess the comment sounds a bit strange now that the default is "true".
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.
Good catch!
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.
Waiting on acting on this until it's confirmed that the switch to the built-in resolvers being on by default should be moved out of this PR.
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.
let's keep the one PR