-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Bump tektoncd/pipeline to v0.12.1 #2260
Conversation
ok to test |
Welcome back 😄 |
@Fabian-K : Could you please review this whenever you find some time? |
Sure, unfortunately I might need 2-3 days to find some time for it :/ |
extensions/tekton/tests/src/test/java/io/fabric8/tekton/test/crud/V1alpha1PipelineCrudTest.java
Outdated
Show resolved
Hide resolved
No rush, take your time |
First quick comment: updating the dependency of generator-v1alpha1 undermines the intention of generating two different models based on different dependency versions. The details are included somewhere in the comments of #2160 (sorry for just throwing a link at you) |
@@ -2,10 +2,17 @@ module github.com/fabric8io/kubernetes-client/extensions/tekton/generator | |||
|
|||
require ( | |||
github.com/fabric8io/kubernetes-client/generator v0.0.0 | |||
github.com/tektoncd/pipeline v0.10.2 |
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.
as mentioned, v1alpha1 should not be updated to the latest version!
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.
Yup I misunderstood it. I thought the v1beta1 client for only v1beta1 resources. I think it has both latest v1alpha1 and v1beta1. While v1alpha client is for old v1alpha1 resources only. In that case, we don't need to update v1alpha1 client then
k8s.io/api => k8s.io/api v0.16.5 | ||
k8s.io/apimachinery => k8s.io/apimachinery v0.16.5 | ||
k8s.io/client-go => k8s.io/client-go v0.16.5 |
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.
Do you know why this is necessary?
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 did this to match with whatever is used in upstream https://github.com/tektoncd/pipeline/blob/release-v0.12.x/go.mod#L63
extensions/tekton/tests/src/test/java/io/fabric8/tekton/test/crud/V1alpha1PipelineCrudTest.java
Outdated
Show resolved
Hide resolved
This will bump tektoncd/pipeline to v0.12.1 for both v1alpha and v1beta1 resources in v1beta1 client. Also use the respective dep as in the upstream project
Kudos, SonarCloud Quality Gate passed! 0 Bugs |
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, thx!
@Fabian-K : Could you please review the update whenever you get time? Once you approve we can proceed to merge this. |
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.
Just one comment, otherwise this LGTM. Thank you!
@@ -6,14 +6,17 @@ cloud.google.com/go v0.44.1/go.mod h1:iSa0KzasP4Uvy3f1mN/7PiObzGgflwredwwASm/v6A | |||
cloud.google.com/go v0.44.2/go.mod h1:60680Gw3Yr4ikxnPRS/oxxkBccT6SA1yMk63TGekxKY= | |||
cloud.google.com/go v0.45.1/go.mod h1:RpBamKRgapWJb87xiFSdk4g1CME7QZg3uwTez+TSTjc= | |||
cloud.google.com/go v0.46.3/go.mod h1:a6bKKbmY7er1mI7TEI4lsAkts/mkhTSZK8w33B4RAg0= | |||
cloud.google.com/go v0.47.0 h1:1JUtpcY9E7+eTospEwWS2QXP3DEn7poB3E2j0jN74mM= |
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´m not sure why there are changes in the go.sum for the v1alpha1. Anyway, they don´t seem to affect the resulting json. Therfore I´m fine with both options: reverting or keeping them
Thanks a lot @piyush-garg ❤️ |
[merge] |
2 similar comments
[merge] |
[merge] |
This will bump tektoncd/pipeline to v0.12.1 for both
v1alpha and v1beta1 resources in v1beta1 client.
Also use the respective dep as in the upstream project