You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@apisix.apache.org by "Chever-John (via GitHub)" <gi...@apache.org> on 2023/04/23 01:19:06 UTC
[GitHub] [apisix-ingress-controller] Chever-John opened a new pull request, #1809: fix: Some CRDs missing status sub-resource
Chever-John opened a new pull request, #1809:
URL: https://github.com/apache/apisix-ingress-controller/pull/1809
<!-- 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 -->
- [x] Bugfix
- [ ] New feature provided
- [ ] Improve performance
- [ ] Backport patches
- [ ] Documentation
- [ ] Refactor
- [ ] Chore
- [ ] CI/CD or Tests
### What this PR does / why we need it:
There are currently several resources in our custom resources that lack an observation configuration for the status sub-resource. And an e2e test file for this function. This PR aims to address these issues and completes the designs and corresponding e2e test files.
<!--- 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
-->
- [x] Did you explain what problem does this PR solve? Or what new features have been added?
- [x] 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**
--
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] Chever-John commented on a diff in pull request #1809: fix: Some CRDs missing status sub-resource
Posted by "Chever-John (via GitHub)" <gi...@apache.org>.
Chever-John commented on code in PR #1809:
URL: https://github.com/apache/apisix-ingress-controller/pull/1809#discussion_r1184953394
##########
test/e2e/suite-ingress/suite-ingress-features/status.go:
##########
@@ -275,6 +276,20 @@ wrw7im4TNSAdwVX4Y1F4svJ2as5SJn5QYGAzXDixNuwzXYrpP9rzA2s=
assert.Contains(ginkgo.GinkgoT(), output, `status: "False"`, "status.conditions.status is recorded")
})
+ ginkgo.It("check ApisixClusterConfig status is recorded", func() {
+ // create ApisixClusterConfig resource
+ clusterConfigName := config.ApisixV2
+ assert.Nil(ginkgo.GinkgoT(), s.NewApisixClusterConfig(clusterConfigName, true, true), "create cluster config error")
+ time.Sleep(6 * time.Second)
+
+ // status should be recorded as successfulen
+ output, err := s.GetOutputFromString("acc", clusterConfigName, "-o", "yaml")
+ assert.Nil(ginkgo.GinkgoT(), err, "Get output of ApisixPluginConfig resource")
+ assert.Contains(ginkgo.GinkgoT(), output, "type: ResourcesAvailable", "status.conditions.type is recorded")
Review Comment:
Yeah, that is a nice choice.
--
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 #1809: fix: Some CRDs missing status sub-resource
Posted by "afzal442 (via GitHub)" <gi...@apache.org>.
afzal442 commented on code in PR #1809:
URL: https://github.com/apache/apisix-ingress-controller/pull/1809#discussion_r1187077079
##########
test/e2e/suite-ingress/suite-ingress-features/status.go:
##########
@@ -275,6 +275,20 @@ wrw7im4TNSAdwVX4Y1F4svJ2as5SJn5QYGAzXDixNuwzXYrpP9rzA2s=
assert.Contains(ginkgo.GinkgoT(), output, `status: "False"`, "status.conditions.status is recorded")
})
+ ginkgo.It("check ApisixClusterConfig status is recorded", func() {
+ // create ApisixClusterConfig resource
+ clusterConfigName := "apisix-cluster-config"
Review Comment:
No issues @Chever-John. It's alright. I am also learning with you. 😄
--
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] Chever-John commented on a diff in pull request #1809: fix: Some CRDs missing status sub-resource
Posted by "Chever-John (via GitHub)" <gi...@apache.org>.
Chever-John commented on code in PR #1809:
URL: https://github.com/apache/apisix-ingress-controller/pull/1809#discussion_r1184849208
##########
test/e2e/suite-ingress/suite-ingress-features/status.go:
##########
@@ -275,6 +275,20 @@ wrw7im4TNSAdwVX4Y1F4svJ2as5SJn5QYGAzXDixNuwzXYrpP9rzA2s=
assert.Contains(ginkgo.GinkgoT(), output, `status: "False"`, "status.conditions.status is recorded")
})
+ ginkgo.It("check ApisixClusterConfig status is recorded", func() {
+ // create ApisixClusterConfig resource
+ clusterConfigName := "cluster-config-name"
+ assert.Nil(ginkgo.GinkgoT(), s.NewApisixClusterConfig(clusterConfigName, true, true), "create cluster config error")
Review Comment:
Did u mean I lose e2e-test for this api(beta3)?
--
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 #1809: fix: Some CRDs missing status sub-resource
Posted by "afzal442 (via GitHub)" <gi...@apache.org>.
afzal442 commented on code in PR #1809:
URL: https://github.com/apache/apisix-ingress-controller/pull/1809#discussion_r1184846849
##########
test/e2e/suite-ingress/suite-ingress-features/status.go:
##########
@@ -275,6 +275,20 @@ wrw7im4TNSAdwVX4Y1F4svJ2as5SJn5QYGAzXDixNuwzXYrpP9rzA2s=
assert.Contains(ginkgo.GinkgoT(), output, `status: "False"`, "status.conditions.status is recorded")
})
+ ginkgo.It("check ApisixClusterConfig status is recorded", func() {
+ // create ApisixClusterConfig resource
+ clusterConfigName := "cluster-config-name"
+ assert.Nil(ginkgo.GinkgoT(), s.NewApisixClusterConfig(clusterConfigName, true, true), "create cluster config error")
Review Comment:
Hey @Chever-John did you check the function definition of NewApisixClus...
Edit:
https://github.com/apache/apisix-ingress-controller/blob/2b31690fc1c4a72c211557f05dbc569bebc60246/test/e2e/scaffold/cluster_config.go#L52
--
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 #1809: fix: Some CRDs missing status sub-resource
Posted by "afzal442 (via GitHub)" <gi...@apache.org>.
afzal442 commented on code in PR #1809:
URL: https://github.com/apache/apisix-ingress-controller/pull/1809#discussion_r1184850063
##########
test/e2e/suite-ingress/suite-ingress-features/status.go:
##########
@@ -275,6 +275,20 @@ wrw7im4TNSAdwVX4Y1F4svJ2as5SJn5QYGAzXDixNuwzXYrpP9rzA2s=
assert.Contains(ginkgo.GinkgoT(), output, `status: "False"`, "status.conditions.status is recorded")
})
+ ginkgo.It("check ApisixClusterConfig status is recorded", func() {
+ // create ApisixClusterConfig resource
+ clusterConfigName := "cluster-config-name"
+ assert.Nil(ginkgo.GinkgoT(), s.NewApisixClusterConfig(clusterConfigName, true, true), "create cluster config error")
Review Comment:
LGTM!
--
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] Chever-John commented on a diff in pull request #1809: fix: Some CRDs missing status sub-resource
Posted by "Chever-John (via GitHub)" <gi...@apache.org>.
Chever-John commented on code in PR #1809:
URL: https://github.com/apache/apisix-ingress-controller/pull/1809#discussion_r1186878140
##########
test/e2e/suite-ingress/suite-ingress-features/status.go:
##########
@@ -275,6 +275,20 @@ wrw7im4TNSAdwVX4Y1F4svJ2as5SJn5QYGAzXDixNuwzXYrpP9rzA2s=
assert.Contains(ginkgo.GinkgoT(), output, `status: "False"`, "status.conditions.status is recorded")
})
+ ginkgo.It("check ApisixClusterConfig status is recorded", func() {
+ // create ApisixClusterConfig resource
+ clusterConfigName := "apisix-cluster-config"
Review Comment:
@afzal442
I apologize for overlooking the fact that the sample on the official website specifies the use of the 'default' value as the default value for metadata.name.
--
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 #1809: fix: Some CRDs missing status sub-resource
Posted by "afzal442 (via GitHub)" <gi...@apache.org>.
afzal442 commented on code in PR #1809:
URL: https://github.com/apache/apisix-ingress-controller/pull/1809#discussion_r1186826445
##########
test/e2e/suite-ingress/suite-ingress-features/status.go:
##########
@@ -275,6 +275,20 @@ wrw7im4TNSAdwVX4Y1F4svJ2as5SJn5QYGAzXDixNuwzXYrpP9rzA2s=
assert.Contains(ginkgo.GinkgoT(), output, `status: "False"`, "status.conditions.status is recorded")
})
+ ginkgo.It("check ApisixClusterConfig status is recorded", func() {
+ // create ApisixClusterConfig resource
+ clusterConfigName := "apisix-cluster-config"
Review Comment:
Hi, since you pass in "default" name to config, you can rmv this `clusterConfigName := "apisix-cluster-config"`
--
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] Chever-John commented on a diff in pull request #1809: fix: Some CRDs missing status sub-resource
Posted by "Chever-John (via GitHub)" <gi...@apache.org>.
Chever-John commented on code in PR #1809:
URL: https://github.com/apache/apisix-ingress-controller/pull/1809#discussion_r1184954875
##########
test/e2e/suite-ingress/suite-ingress-features/status.go:
##########
@@ -275,6 +276,20 @@ wrw7im4TNSAdwVX4Y1F4svJ2as5SJn5QYGAzXDixNuwzXYrpP9rzA2s=
assert.Contains(ginkgo.GinkgoT(), output, `status: "False"`, "status.conditions.status is recorded")
})
+ ginkgo.It("check ApisixClusterConfig status is recorded", func() {
+ // create ApisixClusterConfig resource
+ clusterConfigName := config.ApisixV2
+ assert.Nil(ginkgo.GinkgoT(), s.NewApisixClusterConfig(clusterConfigName, true, true), "create cluster config error")
+ time.Sleep(6 * time.Second)
+
+ // status should be recorded as successfulen
+ output, err := s.GetOutputFromString("acc", clusterConfigName, "-o", "yaml")
+ assert.Nil(ginkgo.GinkgoT(), err, "Get output of ApisixPluginConfig resource")
Review Comment:
tks, I will fix 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] afzal442 commented on a diff in pull request #1809: fix: Some CRDs missing status sub-resource
Posted by "afzal442 (via GitHub)" <gi...@apache.org>.
afzal442 commented on code in PR #1809:
URL: https://github.com/apache/apisix-ingress-controller/pull/1809#discussion_r1184846849
##########
test/e2e/suite-ingress/suite-ingress-features/status.go:
##########
@@ -275,6 +275,20 @@ wrw7im4TNSAdwVX4Y1F4svJ2as5SJn5QYGAzXDixNuwzXYrpP9rzA2s=
assert.Contains(ginkgo.GinkgoT(), output, `status: "False"`, "status.conditions.status is recorded")
})
+ ginkgo.It("check ApisixClusterConfig status is recorded", func() {
+ // create ApisixClusterConfig resource
+ clusterConfigName := "cluster-config-name"
+ assert.Nil(ginkgo.GinkgoT(), s.NewApisixClusterConfig(clusterConfigName, true, true), "create cluster config error")
Review Comment:
Hey @Chever-John did you check the function definition of NewApisixClus...
https://github.com/apache/apisix-ingress-controller/blob/2b31690fc1c4a72c211557f05dbc569bebc60246/pkg/kube/apisix_cluster_config.go#L147
This takes in only obj type.
--
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] Chever-John commented on pull request #1809: fix: Some CRDs missing status sub-resource
Posted by "Chever-John (via GitHub)" <gi...@apache.org>.
Chever-John commented on PR #1809:
URL: https://github.com/apache/apisix-ingress-controller/pull/1809#issuecomment-1714260234
Sure. Pleased to do that~
Let me do it Tomorrow!
--
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] Chever-John commented on a diff in pull request #1809: fix: Some CRDs missing status sub-resource
Posted by "Chever-John (via GitHub)" <gi...@apache.org>.
Chever-John commented on code in PR #1809:
URL: https://github.com/apache/apisix-ingress-controller/pull/1809#discussion_r1184954875
##########
test/e2e/suite-ingress/suite-ingress-features/status.go:
##########
@@ -275,6 +276,20 @@ wrw7im4TNSAdwVX4Y1F4svJ2as5SJn5QYGAzXDixNuwzXYrpP9rzA2s=
assert.Contains(ginkgo.GinkgoT(), output, `status: "False"`, "status.conditions.status is recorded")
})
+ ginkgo.It("check ApisixClusterConfig status is recorded", func() {
+ // create ApisixClusterConfig resource
+ clusterConfigName := config.ApisixV2
+ assert.Nil(ginkgo.GinkgoT(), s.NewApisixClusterConfig(clusterConfigName, true, true), "create cluster config error")
+ time.Sleep(6 * time.Second)
+
+ // status should be recorded as successfulen
+ output, err := s.GetOutputFromString("acc", clusterConfigName, "-o", "yaml")
+ assert.Nil(ginkgo.GinkgoT(), err, "Get output of ApisixPluginConfig resource")
Review Comment:
sry, I will fix 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] Chever-John commented on a diff in pull request #1809: fix: Some CRDs missing status sub-resource
Posted by "Chever-John (via GitHub)" <gi...@apache.org>.
Chever-John commented on code in PR #1809:
URL: https://github.com/apache/apisix-ingress-controller/pull/1809#discussion_r1187080669
##########
test/e2e/suite-ingress/suite-ingress-features/status.go:
##########
@@ -275,6 +275,20 @@ wrw7im4TNSAdwVX4Y1F4svJ2as5SJn5QYGAzXDixNuwzXYrpP9rzA2s=
assert.Contains(ginkgo.GinkgoT(), output, `status: "False"`, "status.conditions.status is recorded")
})
+ ginkgo.It("check ApisixClusterConfig status is recorded", func() {
+ // create ApisixClusterConfig resource
+ clusterConfigName := "apisix-cluster-config"
Review Comment:
Thank you for your previous guidance, it helped me a lot. I was stuck creating a test environment before, but now I can work on this issue more efficiently:)
--
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 #1809: fix: Some CRDs missing status sub-resource
Posted by "afzal442 (via GitHub)" <gi...@apache.org>.
afzal442 commented on code in PR #1809:
URL: https://github.com/apache/apisix-ingress-controller/pull/1809#discussion_r1184937784
##########
test/e2e/suite-ingress/suite-ingress-features/status.go:
##########
@@ -275,6 +275,20 @@ wrw7im4TNSAdwVX4Y1F4svJ2as5SJn5QYGAzXDixNuwzXYrpP9rzA2s=
assert.Contains(ginkgo.GinkgoT(), output, `status: "False"`, "status.conditions.status is recorded")
})
+ ginkgo.It("check ApisixClusterConfig status is recorded", func() {
+ // create ApisixClusterConfig resource
+ clusterConfigName := "cluster-config-name"
+ assert.Nil(ginkgo.GinkgoT(), s.NewApisixClusterConfig(clusterConfigName, true, true), "create cluster config error")
Review Comment:
No it is for apiV1. You are correct. I think you can undo it.
`clusterConfigName := "cluster-config-name"`
--
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 #1809: fix: Some CRDs missing status sub-resource
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #1809:
URL: https://github.com/apache/apisix-ingress-controller/pull/1809#issuecomment-1518987116
## [Codecov](https://codecov.io/gh/apache/apisix-ingress-controller/pull/1809?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 [#1809](https://codecov.io/gh/apache/apisix-ingress-controller/pull/1809?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (f9e3b5e) into [master](https://codecov.io/gh/apache/apisix-ingress-controller/commit/abfacd6ab7ff8129898ef9a1c5e880b92fd52313?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (abfacd6) will **not change** coverage.
> The diff coverage is `n/a`.
> :exclamation: Current head f9e3b5e differs from pull request most recent head e4bcc97. Consider uploading reports for the commit e4bcc97 to get more accurate results
```diff
@@ Coverage Diff @@
## master #1809 +/- ##
=======================================
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] Chever-John commented on a diff in pull request #1809: fix: Some CRDs missing status sub-resource
Posted by "Chever-John (via GitHub)" <gi...@apache.org>.
Chever-John commented on code in PR #1809:
URL: https://github.com/apache/apisix-ingress-controller/pull/1809#discussion_r1186859431
##########
test/e2e/suite-ingress/suite-ingress-features/status.go:
##########
@@ -275,6 +275,20 @@ wrw7im4TNSAdwVX4Y1F4svJ2as5SJn5QYGAzXDixNuwzXYrpP9rzA2s=
assert.Contains(ginkgo.GinkgoT(), output, `status: "False"`, "status.conditions.status is recorded")
})
+ ginkgo.It("check ApisixClusterConfig status is recorded", func() {
+ // create ApisixClusterConfig resource
+ clusterConfigName := "apisix-cluster-config"
Review Comment:
May I know the reason why I should remove this? I didn't pass in any other value for the config name besides 'default'. Is there anything else that I'm missing?
--
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] Chever-John commented on a diff in pull request #1809: fix: Some CRDs missing status sub-resource
Posted by "Chever-John (via GitHub)" <gi...@apache.org>.
Chever-John commented on code in PR #1809:
URL: https://github.com/apache/apisix-ingress-controller/pull/1809#discussion_r1186859431
##########
test/e2e/suite-ingress/suite-ingress-features/status.go:
##########
@@ -275,6 +275,20 @@ wrw7im4TNSAdwVX4Y1F4svJ2as5SJn5QYGAzXDixNuwzXYrpP9rzA2s=
assert.Contains(ginkgo.GinkgoT(), output, `status: "False"`, "status.conditions.status is recorded")
})
+ ginkgo.It("check ApisixClusterConfig status is recorded", func() {
+ // create ApisixClusterConfig resource
+ clusterConfigName := "apisix-cluster-config"
Review Comment:
May I know the reason why I should remove this? I didn't pass in any other value for the config name besides 'apisix-cluster-config'. Is there anything else that I'm missing?
--
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 #1809: fix: Some CRDs missing status sub-resource
Posted by "afzal442 (via GitHub)" <gi...@apache.org>.
afzal442 commented on code in PR #1809:
URL: https://github.com/apache/apisix-ingress-controller/pull/1809#discussion_r1184937784
##########
test/e2e/suite-ingress/suite-ingress-features/status.go:
##########
@@ -275,6 +275,20 @@ wrw7im4TNSAdwVX4Y1F4svJ2as5SJn5QYGAzXDixNuwzXYrpP9rzA2s=
assert.Contains(ginkgo.GinkgoT(), output, `status: "False"`, "status.conditions.status is recorded")
})
+ ginkgo.It("check ApisixClusterConfig status is recorded", func() {
+ // create ApisixClusterConfig resource
+ clusterConfigName := "cluster-config-name"
+ assert.Nil(ginkgo.GinkgoT(), s.NewApisixClusterConfig(clusterConfigName, true, true), "create cluster config error")
Review Comment:
No it is for apiV2. You are correct. I think you can undo it.
`clusterConfigName := "cluster-config-name"`
--
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 #1809: fix: Some CRDs missing status sub-resource
Posted by "afzal442 (via GitHub)" <gi...@apache.org>.
afzal442 commented on code in PR #1809:
URL: https://github.com/apache/apisix-ingress-controller/pull/1809#discussion_r1184953240
##########
test/e2e/suite-ingress/suite-ingress-features/status.go:
##########
@@ -275,6 +276,20 @@ wrw7im4TNSAdwVX4Y1F4svJ2as5SJn5QYGAzXDixNuwzXYrpP9rzA2s=
assert.Contains(ginkgo.GinkgoT(), output, `status: "False"`, "status.conditions.status is recorded")
})
+ ginkgo.It("check ApisixClusterConfig status is recorded", func() {
+ // create ApisixClusterConfig resource
+ clusterConfigName := config.ApisixV2
+ assert.Nil(ginkgo.GinkgoT(), s.NewApisixClusterConfig(clusterConfigName, true, true), "create cluster config error")
+ time.Sleep(6 * time.Second)
+
+ // status should be recorded as successfulen
+ output, err := s.GetOutputFromString("acc", clusterConfigName, "-o", "yaml")
+ assert.Nil(ginkgo.GinkgoT(), err, "Get output of ApisixPluginConfig resource")
Review Comment:
```suggestion
assert.Nil(ginkgo.GinkgoT(), err, "Get output of ApisixClusterConfig resource")
```
--
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
Re: [PR] fix: Some CRDs missing status sub-resource [apisix-ingress-controller]
Posted by "Revolyssup (via GitHub)" <gi...@apache.org>.
Revolyssup commented on PR #1809:
URL: https://github.com/apache/apisix-ingress-controller/pull/1809#issuecomment-1820398594
@Chever-John There are merge conflicts. Are you still working on this?
--
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 #1809: fix: Some CRDs missing status sub-resource
Posted by "afzal442 (via GitHub)" <gi...@apache.org>.
afzal442 commented on code in PR #1809:
URL: https://github.com/apache/apisix-ingress-controller/pull/1809#discussion_r1184951615
##########
test/e2e/suite-ingress/suite-ingress-features/status.go:
##########
@@ -275,6 +276,20 @@ wrw7im4TNSAdwVX4Y1F4svJ2as5SJn5QYGAzXDixNuwzXYrpP9rzA2s=
assert.Contains(ginkgo.GinkgoT(), output, `status: "False"`, "status.conditions.status is recorded")
})
+ ginkgo.It("check ApisixClusterConfig status is recorded", func() {
+ // create ApisixClusterConfig resource
+ clusterConfigName := config.ApisixV2
+ assert.Nil(ginkgo.GinkgoT(), s.NewApisixClusterConfig(clusterConfigName, true, true), "create cluster config error")
+ time.Sleep(6 * time.Second)
+
+ // status should be recorded as successfulen
+ output, err := s.GetOutputFromString("acc", clusterConfigName, "-o", "yaml")
+ assert.Nil(ginkgo.GinkgoT(), err, "Get output of ApisixPluginConfig resource")
+ assert.Contains(ginkgo.GinkgoT(), output, "type: ResourcesAvailable", "status.conditions.type is recorded")
Review Comment:
Did you check the error? It was due to
```
does not contain "type: ResourcesAvailable"
```
You can remove this assert.
--
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 #1809: fix: Some CRDs missing status sub-resource
Posted by "afzal442 (via GitHub)" <gi...@apache.org>.
afzal442 commented on code in PR #1809:
URL: https://github.com/apache/apisix-ingress-controller/pull/1809#discussion_r1184951615
##########
test/e2e/suite-ingress/suite-ingress-features/status.go:
##########
@@ -275,6 +276,20 @@ wrw7im4TNSAdwVX4Y1F4svJ2as5SJn5QYGAzXDixNuwzXYrpP9rzA2s=
assert.Contains(ginkgo.GinkgoT(), output, `status: "False"`, "status.conditions.status is recorded")
})
+ ginkgo.It("check ApisixClusterConfig status is recorded", func() {
+ // create ApisixClusterConfig resource
+ clusterConfigName := config.ApisixV2
+ assert.Nil(ginkgo.GinkgoT(), s.NewApisixClusterConfig(clusterConfigName, true, true), "create cluster config error")
+ time.Sleep(6 * time.Second)
+
+ // status should be recorded as successfulen
+ output, err := s.GetOutputFromString("acc", clusterConfigName, "-o", "yaml")
+ assert.Nil(ginkgo.GinkgoT(), err, "Get output of ApisixPluginConfig resource")
+ assert.Contains(ginkgo.GinkgoT(), output, "type: ResourcesAvailable", "status.conditions.type is recorded")
Review Comment:
Did you check the error? It was due to
```
does not contain "type: ResourcesAvailable"
```
Edit: You can check 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] Chever-John commented on a diff in pull request #1809: fix: Some CRDs missing status sub-resource
Posted by "Chever-John (via GitHub)" <gi...@apache.org>.
Chever-John commented on code in PR #1809:
URL: https://github.com/apache/apisix-ingress-controller/pull/1809#discussion_r1184854448
##########
test/e2e/suite-ingress/suite-ingress-features/status.go:
##########
@@ -275,6 +275,20 @@ wrw7im4TNSAdwVX4Y1F4svJ2as5SJn5QYGAzXDixNuwzXYrpP9rzA2s=
assert.Contains(ginkgo.GinkgoT(), output, `status: "False"`, "status.conditions.status is recorded")
})
+ ginkgo.It("check ApisixClusterConfig status is recorded", func() {
+ // create ApisixClusterConfig resource
+ clusterConfigName := "cluster-config-name"
+ assert.Nil(ginkgo.GinkgoT(), s.NewApisixClusterConfig(clusterConfigName, true, true), "create cluster config error")
Review Comment:
Thanks for pointing out!
--
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 #1809: fix: Some CRDs missing status sub-resource
Posted by "tao12345666333 (via GitHub)" <gi...@apache.org>.
tao12345666333 commented on PR #1809:
URL: https://github.com/apache/apisix-ingress-controller/pull/1809#issuecomment-1700278277
could you please merge the latest code from master branch?
--
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
Re: [PR] fix: Some CRDs missing status sub-resource [apisix-ingress-controller]
Posted by "Revolyssup (via GitHub)" <gi...@apache.org>.
Revolyssup merged PR #1809:
URL: https://github.com/apache/apisix-ingress-controller/pull/1809
--
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] Chever-John commented on a diff in pull request #1809: fix: Some CRDs missing status sub-resource
Posted by "Chever-John (via GitHub)" <gi...@apache.org>.
Chever-John commented on code in PR #1809:
URL: https://github.com/apache/apisix-ingress-controller/pull/1809#discussion_r1186878140
##########
test/e2e/suite-ingress/suite-ingress-features/status.go:
##########
@@ -275,6 +275,20 @@ wrw7im4TNSAdwVX4Y1F4svJ2as5SJn5QYGAzXDixNuwzXYrpP9rzA2s=
assert.Contains(ginkgo.GinkgoT(), output, `status: "False"`, "status.conditions.status is recorded")
})
+ ginkgo.It("check ApisixClusterConfig status is recorded", func() {
+ // create ApisixClusterConfig resource
+ clusterConfigName := "apisix-cluster-config"
Review Comment:
@afzal442
I apologize for overlooking the fact that the sample on the official website specifies the use of the `default` value as the default value for `metadata.name`.
--
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 #1809: fix: Some CRDs missing status sub-resource
Posted by "tao12345666333 (via GitHub)" <gi...@apache.org>.
tao12345666333 commented on PR #1809:
URL: https://github.com/apache/apisix-ingress-controller/pull/1809#issuecomment-1537893635
Please make CI happy first, 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] tao12345666333 commented on a diff in pull request #1809: fix: Some CRDs missing status sub-resource
Posted by "tao12345666333 (via GitHub)" <gi...@apache.org>.
tao12345666333 commented on code in PR #1809:
URL: https://github.com/apache/apisix-ingress-controller/pull/1809#discussion_r1218860605
##########
test/e2e/suite-ingress/suite-ingress-features/status.go:
##########
@@ -234,8 +234,19 @@ wrw7im4TNSAdwVX4Y1F4svJ2as5SJn5QYGAzXDixNuwzXYrpP9rzA2s=
})
//TODO: ApisixGlobal
- //TODO: ApisixConsumer CRD missing status definition
- //TODO: ApisixClusterConfig CRD missing status definition
Review Comment:
Also need to add a test case for the status of ApisixClusterConfig.
--
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] github-actions[bot] commented on pull request #1809: fix: Some CRDs missing status sub-resource
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #1809:
URL: https://github.com/apache/apisix-ingress-controller/pull/1809#issuecomment-1666667315
This pull request has been marked as stale due to 60 days of inactivity. It will be closed in 30 days if no further activity occurs. If you think that's incorrect or this pull request should instead be reviewed, please simply write any comment. Even if closed, you can still revive the PR at any time or discuss it on the dev@apisix.apache.org list. Thank you for your contributions.
--
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
Re: [PR] fix: Some CRDs missing status sub-resource [apisix-ingress-controller]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #1809:
URL: https://github.com/apache/apisix-ingress-controller/pull/1809#issuecomment-1806967656
This pull request has been marked as stale due to 60 days of inactivity. It will be closed in 30 days if no further activity occurs. If you think that's incorrect or this pull request should instead be reviewed, please simply write any comment. Even if closed, you can still revive the PR at any time or discuss it on the dev@apisix.apache.org list. Thank you for your contributions.
--
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] Chever-John commented on pull request #1809: fix: Some CRDs missing status sub-resource
Posted by "Chever-John (via GitHub)" <gi...@apache.org>.
Chever-John commented on PR #1809:
URL: https://github.com/apache/apisix-ingress-controller/pull/1809#issuecomment-1548217241
@AlinsRan need re-run the ci
--
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