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

backport PR2264 into CHE-1818 branch #2288

Merged
merged 2 commits into from
Sep 2, 2016

Conversation

benoitf
Copy link
Contributor

@benoitf benoitf commented Aug 31, 2016

What does this PR do?

Backport changes of PR 2264 into CHE-1818 workspace env refactoring

What issues does this PR fix or reference?

Without this, change in master will be lost

PR type

  • Minor change = no change to existing features or docs

@benoitf
Copy link
Contributor Author

benoitf commented Aug 31, 2016

@garagatyi

@codenvy-ci
Copy link

@garagatyi
Copy link

I will merge CHE-1818 with squash. Are you ok if I'll squash theses changes too?

@@ -162,6 +164,11 @@ public ComposeMachineProviderImpl(DockerConnector docker,

allMachinesSystemVolumes = removeEmptyAndNullValues(allMachinesSystemVolumes);
devMachineSystemVolumes = removeEmptyAndNullValues(devMachineSystemVolumes);

Set<String> volumes = new HashSet<>();
devMachineSystemVolumes.forEach((volume) -> Arrays.asList(volume.split(";")).stream().forEach((entry) -> volumes.add(entry)));

Choose a reason for hiding this comment

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

Consider usage of flatMap instead. I think that this operation can be clearer in that case.

Copy link
Contributor

@skabashnyuk skabashnyuk Sep 1, 2016

Choose a reason for hiding this comment

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

what if?

        devMachineSystemVolumes = devMachineSystemVolumes.stream()
                                                         .map(line -> line.split(";"))
                                                         .flatMap(Arrays::stream)
                                                         .distinct()
                                                         .collect(Collectors.toSet());

@benoitf
Copy link
Contributor Author

benoitf commented Sep 1, 2016

@garagatyi and @skabashnyuk I'm ok to squash everything or not use this PR at all as long as the change about handling multiple volumes with semi colon is handled.

@skabashnyuk as your solution is being more elegant, I've updated PR.

@codenvy-ci
Copy link

Build # 272 - FAILED

Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/272/ to view the results.

@garagatyi
Copy link

AFAIK now it is possible to change base branch of PR. So I think we can wait until CHE-1818 is merged and then change base branch of this PR to master and merge it to master. @benoitf WDYT?

@benoitf
Copy link
Contributor Author

benoitf commented Sep 2, 2016

@garagatyi well current master is having the feature
so technically, merging it without merging first this into CHE-1818 will result in a regression
This is why I wanted to have it merged in what will be applied on master

@benoitf benoitf mentioned this pull request Sep 2, 2016
28 tasks
It is about allowing semi colon mounted Volumes (required for Chefile che-in-che)

Change-Id: I038946a60b4b32609b276d9034be6a15756ac894
Signed-off-by: Florent BENOIT <fbenoit@codenvy.com>
@benoitf benoitf force-pushed the CHE-1818-backport-2264 branch from beaef3a to 71bf958 Compare September 2, 2016 12:29
@tolusha
Copy link
Contributor

tolusha commented Sep 2, 2016

Why not to do the same for allMachinesSystemVolumes ?

@benoitf
Copy link
Contributor Author

benoitf commented Sep 2, 2016

@tolusha I was not sure user was providing something for this variable

@tolusha
Copy link
Contributor

tolusha commented Sep 2, 2016

It will be usefull for codenvy/codenvy#499

…ounted Volumes (required for Chefile che-in-che)
@benoitf
Copy link
Contributor Author

benoitf commented Sep 2, 2016

ok added

@codenvy-ci
Copy link

@garagatyi garagatyi merged commit ad2378a into eclipse-che:CHE-1818 Sep 2, 2016
@benoitf benoitf added this to the 5.0.0-M1 milestone Jan 23, 2017
JPinkney pushed a commit to JPinkney/che that referenced this pull request Aug 17, 2017
Allow semi colon mounted Volumes (required for Chefile che-in-che)

Change-Id: I038946a60b4b32609b276d9034be6a15756ac894
Signed-off-by: Florent BENOIT <fbenoit@codenvy.com>
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.

5 participants