-
Notifications
You must be signed in to change notification settings - Fork 80
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
WIP:Add architecture and os checks when fetching tags #60
Conversation
@tfadeyi: Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it. 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. |
0192ae2
to
66007c1
Compare
66007c1
to
79273fa
Compare
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.
Generally looking good, hope my comments make sense 😄
79273fa
to
0c3371c
Compare
f3c85b3
to
4f8160f
Compare
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.
Looking good, few more comments from me 🙂
@@ -62,7 +62,7 @@ metadata: | |||
name: version-checker | |||
rules: | |||
- apiGroups: [""] | |||
resources: ["pods"] | |||
resources: ["pods","nodes"] |
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.
We also need to update deploy/yaml/deploy.yaml
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 mean the Helm chart?
Isn't the deploy/yaml/deploy.yaml
changed already?
corev1 "k8s.io/api/core/v1" | ||
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
|
||
utils "github.com/jetstack/version-checker/pkg/checker/internal/testutils" |
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.
❤️
nit: can we testutils -> test
8ed61f9
to
5c5d535
Compare
This updates the controller to check the architecture of the cluster nodes and select only tags for the current architecture. Signed-off-by: oluwole.fadeyi <[email protected]>
5c5d535
to
4bab9ef
Compare
/hold |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: JoshVanL, tfadeyi 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 |
@tfadeyi: PR needs rebase. 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. |
Hey @tfadeyi, this has been open a while and needs a rebase at least. Are you interested in updating this branch and getting this feature into version-checker? |
Hey @hawksight I think we could have someone take over, or close this PR. I don't fully remember but there were some edge cases with multi-arch images that this PR won't cover |
This updates the controller to check the architecture of the cluster
nodes and select only tags for the current architecture.
Going to handle the composite hashes in a following PR.
needs: #61
Signed-off-by: oluwole.fadeyi [email protected]