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

If workspace base image does not have labels, skip merge of image labels #7252

Merged
merged 3 commits into from Nov 9, 2017

Conversation

osallou
Copy link
Contributor

@osallou osallou commented Nov 8, 2017

What does this PR do?

This PR fixes issue #7249. If image does not have labels in config, the merge fails with nullpointerexception. This patch does the merge only if labels!=null

What issues does this PR fix or reference?

With SINGLE_MODE, if image does not have any LABEL, an exception is raised when trying to merge the labels. This PR checks that LABEL is not null before merge, else skip the merge.

Release Notes

N/A

Docs PR

None, bug fix only. No impact on documentation.

@osallou osallou requested a review from benoitf as a code owner November 8, 2017 15:43
@codenvy-ci
Copy link

Can one of the admins verify this patch?

1 similar comment
@codenvy-ci
Copy link

Can one of the admins verify this patch?

@benoitf benoitf added the status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. label Nov 8, 2017
@benoitf
Copy link
Contributor

benoitf commented Nov 8, 2017

@osallou you'll need to sign ECA before being able to accept a PR
https://www.eclipse.org/legal/ECA.php

@benoitf
Copy link
Contributor

benoitf commented Nov 8, 2017

ci-build

@osallou
Copy link
Contributor Author

osallou commented Nov 8, 2017

I just signed the ECA, but does not seem to be taken into account in checks...
I also updated commit to be gpg signed, but again ip-validation does not take it into account though commit details show code has been signed (by same address

Relates to eclipse-che#7249
If image does not have labels in config, the merge fails with nullpointerexception.
This patch does the merge only if labels!=null

Signed-off-by: Olivier Sallou <olivier.sallou@gmail.com>
@codenvy-ci
Copy link

Build # 4317 - FAILED

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

Signed-off-by: Olivier Sallou <olivier.sallou@gmail.com>
@benoitf
Copy link
Contributor

benoitf commented Nov 9, 2017

ci-build

@codenvy-ci
Copy link

Build # 4318 - FAILED

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

@benoitf benoitf added the kind/bug Outline of a bug - must adhere to the bug report template. label Nov 9, 2017
@benoitf
Copy link
Contributor

benoitf commented Nov 9, 2017

@osallou could you please run command

$ mvn com.coveo:fmt-maven-plugin:2.0.0:format

in traefik module to fix the formatting issue, thanks

also if you can add a test in https://github.com/osallou/che/blob/231cd2d44b20d3321b9d13ac9f17cd5aade1cdd7/plugins/plugin-traefik/plugin-traefik-docker/src/test/java/org/eclipse/che/plugin/traefik/TraefikCreateContainerInterceptorTest.java

with

when(imageInfoConfig.getLabels()).thenReturn(null);

Signed-off-by: Olivier Sallou <olivier.sallou@gmail.com>
@benoitf
Copy link
Contributor

benoitf commented Nov 9, 2017

ci-build

@codenvy-ci
Copy link

Copy link
Contributor

@benoitf benoitf left a comment

Choose a reason for hiding this comment

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

thanks @osallou !

@benoitf benoitf merged commit d9a79e1 into eclipse-che:master Nov 9, 2017
@benoitf benoitf added this to the 5.21.0 milestone Nov 9, 2017
@benoitf benoitf removed the status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. label Nov 9, 2017
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.

None yet

3 participants