You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@apisix.apache.org by "afzal442 (via GitHub)" <gi...@apache.org> on 2023/04/26 06:32:30 UTC
[GitHub] [apisix-ingress-controller] afzal442 opened a new pull request, #1814: Modified `onUpdate` method
afzal442 opened a new pull request, #1814:
URL: https://github.com/apache/apisix-ingress-controller/pull/1814
<!-- Please answer these questions before submitting a pull request -->
### Type of change:
<!-- Please delete options that are not relevant. -->
<!-- Select all the options from below that matches the type your PR best -->
- [ ] Bugfix
- [ ] New feature provided
- [ ] Improve performance
- [ ] Backport patches
- [ ] Documentation
- [ ] Refactor
- [ ] Chore
- [ ] CI/CD or Tests
### What this PR does / why we need it:
<!--- Why is this change required? What problem does it solve? -->
<!--- If it fixes an open issue, please link to the issue here. -->
### Pre-submission checklist:
<!--
Please follow the requirements:
1. Use Draft if the PR is not ready to be reviewed
2. Test is required for the feat/fix PR, unless you have a good reason
3. Doc is required for the feat PR
4. Use a new commit to resolve review instead of `push -f`
5. Use "request review" to notify the reviewer once you have resolved the review
-->
- [ ] Did you explain what problem does this PR solve? Or what new features have been added?
- [ ] Have you added corresponding test cases?
- [ ] Have you modified the corresponding document?
- [ ] Is this PR backward compatible? **If it is not backward compatible, please discuss on the [mailing list](https://github.com/apache/apisix-ingress-controller#community) first**
cc @tao12345666333
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-ingress-controller] afzal442 closed pull request #1814: Modified `onUpdate` method
Posted by "afzal442 (via GitHub)" <gi...@apache.org>.
afzal442 closed pull request #1814: Modified `onUpdate` method
URL: https://github.com/apache/apisix-ingress-controller/pull/1814
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-ingress-controller] tao12345666333 commented on pull request #1814: Modified `onUpdate` method
Posted by "tao12345666333 (via GitHub)" <gi...@apache.org>.
tao12345666333 commented on PR #1814:
URL: https://github.com/apache/apisix-ingress-controller/pull/1814#issuecomment-1551270890
yes, please!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-ingress-controller] afzal442 commented on pull request #1814: Modified `onUpdate` method
Posted by "afzal442 (via GitHub)" <gi...@apache.org>.
afzal442 commented on PR #1814:
URL: https://github.com/apache/apisix-ingress-controller/pull/1814#issuecomment-1613573762
Closing this as no updates. If required, you can reopen it.
@tao12345666333
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-ingress-controller] afzal442 commented on pull request #1814: Modified `onUpdate` method
Posted by "afzal442 (via GitHub)" <gi...@apache.org>.
afzal442 commented on PR #1814:
URL: https://github.com/apache/apisix-ingress-controller/pull/1814#issuecomment-1549781531
Hey @tao12345666333 ! do you think I will have to write e2e test case for that as there is no?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-ingress-controller] afzal442 commented on pull request #1814: Modified `onUpdate` method
Posted by "afzal442 (via GitHub)" <gi...@apache.org>.
afzal442 commented on PR #1814:
URL: https://github.com/apache/apisix-ingress-controller/pull/1814#issuecomment-1537847728
hi @tao12345666333 as a friendly reminder, can I have some review for this? ⛏️ Are the tests faliing related to it?
One more question, is there e2e-test written for OnAdd() and OnDelete() methods of Gateway controller? Thanks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-ingress-controller] codecov-commenter commented on pull request #1814: Modified `onUpdate` method
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #1814:
URL: https://github.com/apache/apisix-ingress-controller/pull/1814#issuecomment-1525101774
## [Codecov](https://codecov.io/gh/apache/apisix-ingress-controller/pull/1814?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#1814](https://codecov.io/gh/apache/apisix-ingress-controller/pull/1814?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (3eed87e) into [master](https://codecov.io/gh/apache/apisix-ingress-controller/commit/2182a48cbca785373eca745a13d8cf2b7d9ab6c8?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (2182a48) will **not change** coverage.
> The diff coverage is `n/a`.
> :exclamation: Current head 3eed87e differs from pull request most recent head ba87b5f. Consider uploading reports for the commit ba87b5f to get more accurate results
```diff
@@ Coverage Diff @@
## master #1814 +/- ##
=======================================
Coverage 39.45% 39.45%
=======================================
Files 91 91
Lines 8043 8043
=======================================
Hits 3173 3173
Misses 4463 4463
Partials 407 407
```
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-ingress-controller] afzal442 commented on pull request #1814: Modified `onUpdate` method
Posted by "afzal442 (via GitHub)" <gi...@apache.org>.
afzal442 commented on PR #1814:
URL: https://github.com/apache/apisix-ingress-controller/pull/1814#issuecomment-1526028801
Hi! looks like there are already test cases https://github.com/apache/apisix-ingress-controller/blob/2182a48cbca785373eca745a13d8cf2b7d9ab6c8/test/e2e/suite-features/external-sd.go#L235
And I think we have test failed due to this https://github.com/apache/apisix-ingress-controller/blob/2182a48cbca785373eca745a13d8cf2b7d9ab6c8/test/e2e/suite-features/external-service.go#L289
I will try again with `make e2e-test-local`. Any thoughts?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-ingress-controller] afzal442 commented on pull request #1814: Modified `onUpdate` method
Posted by "afzal442 (via GitHub)" <gi...@apache.org>.
afzal442 commented on PR #1814:
URL: https://github.com/apache/apisix-ingress-controller/pull/1814#issuecomment-1525239065
Sure! I will try to do that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-ingress-controller] afzal442 commented on a diff in pull request #1814: Modified `onUpdate` method
Posted by "afzal442 (via GitHub)" <gi...@apache.org>.
afzal442 commented on code in PR #1814:
URL: https://github.com/apache/apisix-ingress-controller/pull/1814#discussion_r1179348621
##########
pkg/providers/gateway/gateway.go:
##########
@@ -206,6 +207,49 @@ func (c *gatewayController) onAdd(obj interface{}) {
}
func (c *gatewayController) onUpdate(oldObj, newObj interface{}) {
+ oldGateway, ok := oldObj.(*gatewayv1beta1.Gateway)
+ if !ok {
+ log.Errorw("failed to convert old object to gateway",
+ zap.Any("obj", oldObj),
+ )
+ return
+ }
+
+ newGateway, ok := newObj.(*gatewayv1beta1.Gateway)
+ if !ok {
+ log.Errorw("failed to convert new object to gateway",
+ zap.Any("new obj", newObj),
+ )
+ return
+ }
+
+ if reflect.DeepEqual(oldGateway.Spec, newGateway.Spec) {
Review Comment:
```suggestion
if oldHTTPRoute.ResourceVersion >= newHTTPRoute.ResourceVersion {
```
I just wanted to know if we can change it to this one or previous one is okay.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-ingress-controller] afzal442 commented on a diff in pull request #1814: Modified `onUpdate` method
Posted by "afzal442 (via GitHub)" <gi...@apache.org>.
afzal442 commented on code in PR #1814:
URL: https://github.com/apache/apisix-ingress-controller/pull/1814#discussion_r1179348621
##########
pkg/providers/gateway/gateway.go:
##########
@@ -206,6 +207,49 @@ func (c *gatewayController) onAdd(obj interface{}) {
}
func (c *gatewayController) onUpdate(oldObj, newObj interface{}) {
+ oldGateway, ok := oldObj.(*gatewayv1beta1.Gateway)
+ if !ok {
+ log.Errorw("failed to convert old object to gateway",
+ zap.Any("obj", oldObj),
+ )
+ return
+ }
+
+ newGateway, ok := newObj.(*gatewayv1beta1.Gateway)
+ if !ok {
+ log.Errorw("failed to convert new object to gateway",
+ zap.Any("new obj", newObj),
+ )
+ return
+ }
+
+ if reflect.DeepEqual(oldGateway.Spec, newGateway.Spec) {
Review Comment:
```suggestion
if oldGateway.ResourceVersion >= newGateway.ResourceVersion {
```
I just wanted to know if we can change it to this one or previous one is okay.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-ingress-controller] tao12345666333 commented on pull request #1814: Modified `onUpdate` method
Posted by "tao12345666333 (via GitHub)" <gi...@apache.org>.
tao12345666333 commented on PR #1814:
URL: https://github.com/apache/apisix-ingress-controller/pull/1814#issuecomment-1525089504
Thanks for your contributions.
Please add an e2e test case for it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-ingress-controller] tao12345666333 commented on pull request #1814: Modified `onUpdate` method
Posted by "tao12345666333 (via GitHub)" <gi...@apache.org>.
tao12345666333 commented on PR #1814:
URL: https://github.com/apache/apisix-ingress-controller/pull/1814#issuecomment-1537891874
Not yet.
All tests related to Gateway API are stored in this directory: https://github.com/apache/apisix-ingress-controller/tree/master/test/e2e/suite-gateway
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org