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

Do not apply proxy settings for JWTProxy container; #14448

Merged
merged 2 commits into from
Sep 9, 2019
Merged

Conversation

mshaposhnik
Copy link
Contributor

@mshaposhnik mshaposhnik commented Sep 6, 2019

What does this PR do?

When workspace proxy is set, this setting applied to all containers of the workspace, including JWTProxy. So when JWTProxy tries to access the upstream endpoint (which is always in internal form, e.g. http://servers7ggiqm3-che-machine-execs37:4444) it fails. This PR prevents of adding proxy settings to the JWTProxy containers, which is OK since JWTProxy never does any outbound requests.

What issues does this PR fix or reference?

#14052

Release Notes

N/A

Docs PR

N/A

Signed-off-by: Max Shaposhnik <mshaposh@redhat.com>
@che-bot che-bot added status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. kind/bug Outline of a bug - must adhere to the bug report template. labels Sep 6, 2019
@che-bot
Copy link
Contributor

che-bot commented Sep 6, 2019

E2E Happy path tests of Eclipse Che Single User on K8S (minikube v1.1.1) has been successful:

  • build details
  • "che-server" docker image: maxura/che-server:14448

Copy link
Member

@sleshchenko sleshchenko left a comment

Choose a reason for hiding this comment

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

LGTM

.getPodsData()
.entrySet()
.stream()
.filter(entry -> !entry.getKey().equals(JWT_PROXY_POD_NAME))
Copy link
Member

Choose a reason for hiding this comment

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

  1. Please consider adding one like comment why here we ignore JWT_PROXY pod.

  2. Please consider adding test case for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why i am always modifying classes which doesn;t have tests at all 🤣 ?

Copy link
Member

Choose a reason for hiding this comment

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

Cause you're a lucky man! 🤣

@che-bot
Copy link
Contributor

che-bot commented Sep 6, 2019

E2E tests of Eclipse Che Multiuser on OCP has failed:

Copy link
Contributor

@amisevsk amisevsk left a comment

Choose a reason for hiding this comment

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

LGTM

Signed-off-by: Max Shaposhnik <mshaposh@redhat.com>
@mshaposhnik
Copy link
Contributor Author

ci-test

@che-bot
Copy link
Contributor

che-bot commented Sep 6, 2019

E2E Happy path tests of Eclipse Che Single User on K8S (minikube v1.1.1) has been successful:

  • build details
  • "che-server" docker image: maxura/che-server:14448

@che-bot
Copy link
Contributor

che-bot commented Sep 6, 2019

E2E tests of Eclipse Che Multiuser on OCP has failed:

@mshaposhnik
Copy link
Contributor Author

ci-build

Copy link
Contributor

@metlos metlos left a comment

Choose a reason for hiding this comment

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

LGTM

@artaleks9
Copy link
Contributor

Selenium tests execution on Eclipse Che Multiuser on OCP (https://ci.codenvycorp.com/job/che-pullrequests-test-ocp/2217/) doesn't show any regression against this Pull Request.

@mshaposhnik mshaposhnik merged commit f8db913 into master Sep 9, 2019
@mshaposhnik mshaposhnik deleted the CHE-14052 branch September 9, 2019 08:40
@che-bot che-bot removed the status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. label Sep 9, 2019
@che-bot che-bot added this to the 7.2.0 milestone Sep 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Outline of a bug - must adhere to the bug report template.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants