You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@camel.apache.org by "gansheer (via GitHub)" <gi...@apache.org> on 2024/06/04 14:46:48 UTC
[PR] fix(e2e): Ensures Maven proxy test is not infra dependant [camel-k]
gansheer opened a new pull request, #5581:
URL: https://github.com/apache/camel-k/pull/5581
Modifications on maven http proxy test modification to avoid infra configuration dependance:
* on passing test remove the kit copy and the CONNECT log verification on OCP: integration kit builds with proxy configuration should pass
* add a non passing test with a non existant proxy: integration kit build should fail
Both test should be enought to prove the maven proxy test works.
<!--
Enter your extended release note in the below block. If the PR requires
additional action from users switching to the new release, include the string
"action required". If no release note is required, write "NONE".
You can (optionally) mark this PR with labels "kind/bug" or "kind/feature" to make sure
the text is added to the right section of the release notes.
-->
**Release Note**
```release-note
fix(e2e): Ensures Maven proxy test is not infra dependant
```
--
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: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(e2e): Ensures Maven proxy test is not infra dependant [camel-k]
Posted by "gansheer (via GitHub)" <gi...@apache.org>.
gansheer commented on PR #5581:
URL: https://github.com/apache/camel-k/pull/5581#issuecomment-2149070051
> LGTM. However I'd try to make an effort to remove platform specific conditions at all when we find them. We are already suffering this legacy situation and have difficulty to maintain the whole codebase. IMO, the negative test could be enough to prove that, when using a non existing proxy, the operator cannot build, therefore we may get rid off the "CONNECT" check condition.
--
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: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(e2e): Ensures Maven proxy test is not infra dependant [camel-k]
Posted by "squakez (via GitHub)" <gi...@apache.org>.
squakez commented on code in PR #5581:
URL: https://github.com/apache/camel-k/pull/5581#discussion_r1627207818
##########
e2e/advanced/maven_http_proxy_test.go:
##########
@@ -210,10 +210,15 @@ func TestMavenProxy(t *testing.T) {
g.Expect(err).To(Succeed())
g.Expect(proxies.Items).To(HaveLen(1))
+ ocp, err := openshift.IsOpenShift(TestClient(t))
+ require.NoError(t, err)
+
logs := Logs(t, ctx, ns, proxies.Items[0].Name, corev1.PodLogOptions{})()
g.Expect(logs).NotTo(BeEmpty())
- g.Expect(logs).To(ContainSubstring("\"CONNECT repo.maven.apache.org:443 HTTP/1.1\" 200"))
-
+ if !ocp {
+ // OpenShift has internal configuration (like oauth) that overriddes the default repo connect URL
+ g.Expect(logs).To(ContainSubstring("\"CONNECT repo.maven.apache.org:443 HTTP/1.1\" 200"))
Review Comment:
I think that's fine. What we should understand is if there is anything specific in the operator log that can make us check that we're connecting to a proxy. However, this could be something done at OS level, so, I think the reasoning on removing the check is fine by assuming that any third party will do its work. In theory, we should be fine by setting the env var and any tooling should be honoring such a configuration. However, Maven is a bit particular, and it requires an additional setting [1]. This is in fact something we are already doing, see:
https://github.com/apache/camel-k/blob/cb74737f23c8622a100eaa67a03afc6ca6f1ac35/pkg/util/maven/maven_proxies.go#L30
A follow up work we can do (into this or after this PR) is to add some unit test to validate that, in presence of the HTTP_PROXY vars, the settings are created as expected. And the E2E can validate the presence of such configuration in the maven settings created next to the IntegrationPlatform.
[1] https://maven.apache.org/guides/mini/guide-proxies.html
--
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: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(e2e): Ensures Maven proxy test is not infra dependant [camel-k]
Posted by "gansheer (via GitHub)" <gi...@apache.org>.
gansheer commented on code in PR #5581:
URL: https://github.com/apache/camel-k/pull/5581#discussion_r1627149760
##########
e2e/advanced/maven_http_proxy_test.go:
##########
@@ -210,10 +210,15 @@ func TestMavenProxy(t *testing.T) {
g.Expect(err).To(Succeed())
g.Expect(proxies.Items).To(HaveLen(1))
+ ocp, err := openshift.IsOpenShift(TestClient(t))
+ require.NoError(t, err)
+
logs := Logs(t, ctx, ns, proxies.Items[0].Name, corev1.PodLogOptions{})()
g.Expect(logs).NotTo(BeEmpty())
- g.Expect(logs).To(ContainSubstring("\"CONNECT repo.maven.apache.org:443 HTTP/1.1\" 200"))
-
+ if !ocp {
+ // OpenShift has internal configuration (like oauth) that overriddes the default repo connect URL
+ g.Expect(logs).To(ContainSubstring("\"CONNECT repo.maven.apache.org:443 HTTP/1.1\" 200"))
Review Comment:
I removed the check but kept the passing test. I think proof of build with the proxy present goes by hand with the build error without the proxy. The proxy being test code driven makes it less infra dependant.
But if you think it is too much to maintain I can remove it as well.
--
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: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(e2e): Ensures Maven proxy test is not infra dependant [camel-k]
Posted by "gansheer (via GitHub)" <gi...@apache.org>.
gansheer commented on code in PR #5581:
URL: https://github.com/apache/camel-k/pull/5581#discussion_r1627461820
##########
e2e/advanced/maven_http_proxy_test.go:
##########
@@ -210,10 +210,15 @@ func TestMavenProxy(t *testing.T) {
g.Expect(err).To(Succeed())
g.Expect(proxies.Items).To(HaveLen(1))
+ ocp, err := openshift.IsOpenShift(TestClient(t))
+ require.NoError(t, err)
+
logs := Logs(t, ctx, ns, proxies.Items[0].Name, corev1.PodLogOptions{})()
g.Expect(logs).NotTo(BeEmpty())
- g.Expect(logs).To(ContainSubstring("\"CONNECT repo.maven.apache.org:443 HTTP/1.1\" 200"))
-
+ if !ocp {
+ // OpenShift has internal configuration (like oauth) that overriddes the default repo connect URL
+ g.Expect(logs).To(ContainSubstring("\"CONNECT repo.maven.apache.org:443 HTTP/1.1\" 200"))
Review Comment:
Good ideas.
Let's merge this PR as it is and I will be adding some UT and improve the e2e configuration check in another PR.
--
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: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(e2e): Ensures Maven proxy test is not infra dependant [camel-k]
Posted by "squakez (via GitHub)" <gi...@apache.org>.
squakez commented on code in PR #5581:
URL: https://github.com/apache/camel-k/pull/5581#discussion_r1627023510
##########
e2e/advanced/maven_http_proxy_test.go:
##########
@@ -210,10 +210,15 @@ func TestMavenProxy(t *testing.T) {
g.Expect(err).To(Succeed())
g.Expect(proxies.Items).To(HaveLen(1))
+ ocp, err := openshift.IsOpenShift(TestClient(t))
+ require.NoError(t, err)
+
logs := Logs(t, ctx, ns, proxies.Items[0].Name, corev1.PodLogOptions{})()
g.Expect(logs).NotTo(BeEmpty())
- g.Expect(logs).To(ContainSubstring("\"CONNECT repo.maven.apache.org:443 HTTP/1.1\" 200"))
-
+ if !ocp {
+ // OpenShift has internal configuration (like oauth) that overriddes the default repo connect URL
+ g.Expect(logs).To(ContainSubstring("\"CONNECT repo.maven.apache.org:443 HTTP/1.1\" 200"))
Review Comment:
I'd avoid filling our code (also test code) with platform specific conditions. I think we need to find a general way to make a test.
--
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: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(e2e): Ensures Maven proxy test is not infra dependant [camel-k]
Posted by "gansheer (via GitHub)" <gi...@apache.org>.
gansheer closed pull request #5581: fix(e2e): Ensures Maven proxy test is not infra dependant
URL: https://github.com/apache/camel-k/pull/5581
--
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: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(e2e): Ensures Maven proxy test is not infra dependant [camel-k]
Posted by "gansheer (via GitHub)" <gi...@apache.org>.
gansheer merged PR #5581:
URL: https://github.com/apache/camel-k/pull/5581
--
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: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org