Skip to content

Conversation

@micnncim
Copy link

@micnncim micnncim commented Nov 16, 2020

Added a short name to EnvoyFilter as well as other CRDs.

@istio-policy-bot
Copy link

😊 Welcome @micnncim! This is either your first contribution to the Istio api repo, or it's been
awhile since you've been here.

You can learn more about the Istio working groups, code of conduct, and contributing guidelines
by referring to Contributing to Istio.

Thanks for contributing!

Courtesy of your friendly welcome wagon.

@istio-testing istio-testing added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Nov 16, 2020
@google-cla google-cla bot added the cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. label Nov 16, 2020
@istio-testing
Copy link
Collaborator

Hi @micnncim. Thanks for your PR.

I'm waiting for a istio member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

Copy link
Member

@howardjohn howardjohn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@hzxuzhonghu
Copy link
Member

/ok-to-test

@istio-testing istio-testing added ok-to-test Set this label allow normal testing to take place for a PR not submitted by an Istio org member. and removed needs-ok-to-test labels Nov 17, 2020
@costinm
Copy link
Contributor

costinm commented Nov 17, 2020

Should we avoid taking over 2-letter names for alpha APIs ? I'm not sure how k8s reacts if different products register short names,
and we may already have a problem with the k8s Gateways if they start registering short name.

At least the 'short name' should have a failsafe setting, I suspect install would fail if there is a conflict ( I'm more concerned of what will happen with gateways for this @howardjohn )

Not blocking this, just thinking out loud.

@howardjohn
Copy link
Member

howardjohn commented Nov 17, 2020 via email

@micnncim
Copy link
Author

micnncim commented Nov 17, 2020

Thank you for point out the issue.

I've checked the related issues and PRs (kubernetes/kubernetes#94860, kubernetes-sigs/gateway-api#426).

The best solution would be that k8s takes care of this issue like warning, right? Because we can't expect a conflict with other CRDs completely.

However, since it's not certain k8s will take care of it, would it be better to take more than 2 letter word like evf, efl? IMO, typing something like kubectl get envoyfilter every time would be annoying for some of the users.

@smawson
Copy link
Contributor

smawson commented Nov 17, 2020

I wonder if we should have a policy against adding short names to alpha apis? Seems like it could be done as part of moving an api to beta. Thoughts?

@micnncim
Copy link
Author

/retest

@istio-testing
Copy link
Collaborator

@micnncim: The following test failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
release-notes_api 0142fcb link /test release-notes_api

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. I understand the commands that are listed here.

@micnncim
Copy link
Author

/release-note-none

@hzxuzhonghu
Copy link
Member

All our APIs are alpha now, there isn't any harm even conflict with other CRs, is there?

@istio-testing istio-testing added the needs-rebase Indicates a PR needs to be rebased before being merged label Mar 4, 2021
@istio-testing
Copy link
Collaborator

@micnncim: 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.

@howardjohn howardjohn removed the cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. label May 15, 2024
@istio-policy-bot
Copy link

🚧 This issue or pull request has been closed due to not having had activity from an Istio team member since 2020-11-18. If you feel this issue or pull request deserves attention, please reopen the issue. Please see this wiki page for more information. Thank you for your contributions.

Created by the issue and PR lifecycle manager.

@istio-policy-bot istio-policy-bot added the lifecycle/automatically-closed Indicates a PR or issue that has been closed automatically. label May 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

lifecycle/automatically-closed Indicates a PR or issue that has been closed automatically. needs-rebase Indicates a PR needs to be rebased before being merged ok-to-test Set this label allow normal testing to take place for a PR not submitted by an Istio org member. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants