Skip to content
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

Fix degraded proxy support for http(s) git repository (#2243) #2249

Merged
merged 1 commit into from
Sep 5, 2019
Merged

Fix degraded proxy support for http(s) git repository (#2243) #2249

merged 1 commit into from
Sep 5, 2019

Conversation

mitsutaka
Copy link
Contributor

Signed-off-by: Mitz Amano mitz@linux.com

Fix degraded proxy support for https(s) git repository. solves #2243

Signed-off-by: Mitz Amano <mitz@linux.com>
@CLAassistant
Copy link

CLAassistant commented Sep 5, 2019

CLA assistant check
All committers have signed the CLA.

@mitsutaka
Copy link
Contributor Author

I don't have yet idea how to implement e2e test for proxy/

Copy link
Member

@jannfis jannfis left a comment

Choose a reason for hiding this comment

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

Thank you for this PR! The change looks good to me, and I also like the optimization in HTTP client settings.

Regarding the E2E tests: I guess this is a somewhat bigger operation. We'd have to integrate a HTTP proxy server into the argo-cd-ci-builder Docker image (@alexmt I'm struggling to find the Dockerfile for this, can you give us a pointer please?) started from test/fixture/testrepos, and then adapt/extend the tests and test fixtures in test/e2e so that we test at least one repository connection via the HTTP proxy.

However, from my point of view, this fix is small enough so it can be incorporated without an existing E2E test and we'll provide such a test later on, so that we can avoid regress in the future.

@mitsutaka
Copy link
Contributor Author

Thank you for approval. I agree with you that the enhancement of the e2e test environment would spend more time.

Copy link
Contributor

@alexec alexec left a comment

Choose a reason for hiding this comment

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

I'll approve as @jannfis does not have perms.

@alexec alexec merged commit e322750 into argoproj:master Sep 5, 2019
@alexec alexec added this to the v1.3 milestone Sep 5, 2019
@mitsutaka mitsutaka deleted the fix-degraded-proxy branch September 5, 2019 22:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants