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.

Sign up for GitHub

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

Jump to bottom

Provision git configuration into each container #14402

Merged
merged 1 commit into from
Sep 4, 2019
Merged

Provision git configuration into each container #14402

merged 1 commit into from
Sep 4, 2019

Conversation

vzhukovs
Copy link
Contributor

@vzhukovs vzhukovs commented Sep 2, 2019 •
edited
Loading

What does this PR do?

This changes proposal adds ability to provision git configuration into each container at container startup. Configuration consists of user name and user email needed for git to sign user commits.

Linked PR on the Theia side, which provide preference editor to configure the values operated in this PR: eclipse-che/che-theia#424

Demo for this PR: https://www.youtube.com/watch?v=Q_XsUWR_mJY&feature=youtu.be

Signed-off-by: Vlad Zhukovskyi vzhukovs@redhat.com

What issues does this PR fix or reference?

#13874

Release Notes

Provision git configuration into each container

Docs PR

@che-bot che-bot added kind/enhancement A feature request - must adhere to the feature request template. status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. labels Sep 2, 2019
@che-bot
Copy link
Contributor

che-bot commented Sep 2, 2019

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

@che-bot
Copy link
Contributor

che-bot commented Sep 2, 2019

E2E tests of Eclipse Che Multiuser on OCP has failed:

@che-bot
Copy link
Contributor

che-bot commented Sep 2, 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:14402

@che-bot
Copy link
Contributor

che-bot commented Sep 2, 2019

E2E tests of Eclipse Che Multiuser on OCP has failed:

@vzhukovs vzhukovs self-assigned this Sep 2, 2019
@vzhukovs vzhukovs added this to the 7.1.0 milestone Sep 2, 2019
@vzhukovs vzhukovs mentioned this pull request Sep 2, 2019
Merged
gorkem
gorkem reviewed Sep 2, 2019
View reviewed changes
...org/eclipse/che/workspace/infrastructure/kubernetes/provision/GitUserProfileProvisioner.java Outdated
String userId = EnvironmentContext.getCurrent().getSubject().getUserId();
Map<String, String> preferencesMap = preferenceManager.find(userId, keyFilter);

return ofNullable(preferencesMap.get(PREFERENCES_KEY_FILTER));
Copy link
Contributor

@gorkem gorkem Sep 2, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

This function is a bit odd for me. It is a private function that is used with PREFERENCES_KEY_FILTER as keyFilter only. And in this line it refers to PREFERENCES_KEY_FILTER directly. I feel that either it should not refer to PREFERENCES_KEY_FILTER or not have a keyFilter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, you're right! Thanks for the pointing. keyFilter argument actually has the same value as PREFERENCES_KEY_FILTER has. Will replace constant value with argument value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also will update name of the function, getPreferenceValue is more preferred name for it.

@che-bot
Copy link
Contributor

che-bot commented Sep 2, 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:14402

@che-bot
Copy link
Contributor

che-bot commented Sep 2, 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:14402

@che-bot
Copy link
Contributor

che-bot commented Sep 2, 2019

E2E tests of Eclipse Che Multiuser on OCP has failed:

amisevsk
amisevsk approved these changes Sep 3, 2019
View reviewed changes
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

...org/eclipse/che/workspace/infrastructure/kubernetes/provision/GitUserProfileProvisioner.java
import static java.util.Collections.singletonMap;
import static java.util.Optional.empty;
import static java.util.Optional.of;
import static java.util.Optional.ofNullable;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not a fan of static imports of methods such as these, since names like of are not descriptive at all. Up to you though, I know we do it all over the codebase currently.

@vzhukovs
Provision git configuration into each container
6b4bd00 
Signed-off-by: Vlad Zhukovskyi <vzhukovs@redhat.com>
@che-bot
Copy link
Contributor

che-bot commented Sep 3, 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:14402

@che-bot
Copy link
Contributor

che-bot commented Sep 3, 2019

E2E tests of Eclipse Che Multiuser on OCP has failed:

@SkorikSergey
Copy link
Contributor

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

@vzhukovs vzhukovs merged commit 25de692 into master Sep 4, 2019
@vzhukovs vzhukovs deleted the che#13874 branch September 4, 2019 08:37
@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 4, 2019
@sleshchenko sleshchenko mentioned this pull request Sep 5, 2019
Closed
6 tasks
amisevsk
amisevsk reviewed Sep 5, 2019
View reviewed changes
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.

@vzhukovskii I was testing these changes on OpenShift, and it looks like the provisioning isn't included in OpenShiftEnvironmentProvisioner.

@vzhukovs
Copy link
Contributor Author

vzhukovs commented Sep 5, 2019

@amisevsk double checked everything, and you're right. Registration in OpenShiftEnvironmentProvisioner is missing. I'll add registration in #14442 and recheck, how it goes on OpenShift.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gorkem gorkem gorkem left review comments

@amisevsk amisevsk amisevsk approved these changes

@azatsarynnyy azatsarynnyy Awaiting requested review from azatsarynnyy

@vparfonov vparfonov Awaiting requested review from vparfonov

@l0rd l0rd Awaiting requested review from l0rd

@metlos metlos Awaiting requested review from metlos

@nickboldt nickboldt Awaiting requested review from nickboldt

@rhopp rhopp Awaiting requested review from rhopp

@sleshchenko sleshchenko Awaiting requested review from sleshchenko

Assignees

@vzhukovs vzhukovs

Labels
kind/enhancement A feature request - must adhere to the feature request template.
Projects
None yet
Milestone
7.1.0
Development

Successfully merging this pull request may close these issues.
5 participants
@vzhukovs @che-bot @SkorikSergey @gorkem @amisevsk