-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
✨ (kustomize/v1) add validation to let users know when the architecture used to run the plugin is not supported #2698
✨ (kustomize/v1) add validation to let users know when the architecture used to run the plugin is not supported #2698
Conversation
c375eea
to
4b96e1d
Compare
/test pull-kubebuilder-e2e-k8s-1-16-15 |
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.
First round. I like the idea of improving the error message to be more descriptive!
3d4bb04
to
7873b85
Compare
@everettraven thank you for the excellent review! |
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.
A couple minor nits and a question regarding the x86_64
replaces
/label tide/merge-method-squash |
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
@everettraven: changing LGTM is restricted to collaborators 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. |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: camilamacedo86, everettraven 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 |
216963a
to
fd4765c
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.
Just some nits :)
supportedArchs) | ||
} | ||
|
||
return nil |
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.
It seems the error return of this function will always be nil?
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.
Yes, because we will never return an error here so far.
See that it is an implementation of the interface:
// PreScaffold executes tasks before the main scaffolding.
PreScaffold(machinery.Filesystem) error
…re used to run the plugin is not supported Co-authored-by: Bryce Palmer <everettraven@gmail.com>
fd4765c
to
784e31e
Compare
Hi @FillZpp, Thank you for the review. 🥇 |
/lgtm |
Description
(kustomize/v1) add validation to let users know when the architecture used to run the plugin is not supported
Motivation
We have seen many users get stuck. So, the idea here is clarifying for contributors, KB lib consumers and end-users what are the current support architectures by kustomize/v1. Example: operator-framework/operator-sdk#5785