fix: dont NPE if initializing odo fails (#536)

[adietish]

fixes #536

[adietish]

@sbouchet this PR has 2 commits:

• 1st commit: simple fix
• 2nd commit: change in how odo is downloaded if it doesnt exist yet
  Would love to get your take on this 2nd commit which I believe is an improvement/simplification of the whole approach

The 2nd commit tries to achieve the following:
When using the odo binary the tooling would possibly download it first. You can see it here:
https://github.com/redhat-developer/intellij-openshift-connector/pull/589/files#diff-7e1e10d56d5a6eeb30015061c61[…]e481e2f817d81e3d2457647157R77-R84

    public CompletableFuture<Odo> getOdo() {
        if (odoFuture == null) {
            this.odoFuture = OdoCliFactory.getInstance()
              .getOdo(project)
              .whenComplete((odo, err) -> structure.fireModified(this));
        }
        return odoFuture;
    }

The existing code does this differently. The ApplicationsRootNode holds a reference to odo. Odo is the facade for the binary. ApplicaitonsRootNode loads and stores the odo facade once it downloaded the binary:
https://github.com/redhat-developer/intellij-openshift-connector/pull/589/commits/77199886cdd754a50d57ae85399433292547b389#diff-7e1e10[…]7157R78-R85
Possibly downloading the binary has to be triggered explicitly:

    public CompletableFuture<Odo> initializeOdo() {
        return OdoCliFactory.getInstance().getOdo(project).whenComplete((odo, err) -> {
            if (this.odo != null) {
                this.odo.release();
        return OdoCliFactory.getInstance().getOdo(project).handle((odo, e) -> {
            if (e != null) {
                setError(new RuntimeException("Could not initialize Odo", e.getCause()));
                return null;
            } else {
                setOdo(odo);
                return odo;
            }

There are 2 things that I dont like here:

1. you need to explicitly call initializeOdo so that the odo binary gets possibly downladed
2. once it’s loaded the odo binary and a possible error is set to the ApplicationsRootNode

I tried to address this as follows:

1. get rid of explicitly calling initializeOdo() . Load it transparently whenever getOdo() is called
2. get rid of the instance var odo and error. Replace by an instance var odoFuture which is a CompletableFuture. It holds odo and the possible error.

As a result of this change any time user code wants to use the odo binary it has to try/catch to get errors that may have happened when loading the binary and null-check in case the odo binary wasnt loaded yet:

 try {
     Odo odo = componentNode.getRoot().getOdo().getNow(null);
     if (odo == null) {
         return;
     }
     ...
 } catch (Exception ex) {

[sbouchet]

I tried to address this as follows:

1. get rid of explicitly calling initializeOdo() . Load it transparently whenever getOdo() is called
2. get rid of the instance var odo and error. Replace by an instance var odoFuture which is a CompletableFuture. It holds odo and the possible error.

As a result of this change any time user code wants to use the odo binary it has to try/catch to get errors that may have happened when loading the binary and null-check in case the odo binary wasnt loaded yet:

 try {
     Odo odo = componentNode.getRoot().getOdo().getNow(null);
     if (odo == null) {
         return;
     }
     ...
 } catch (Exception ex) {

I like the idea to just call getOdo() that returns a future. this will also simplyfy the codebase by removing the odoprojectdecorator class. +1 for implement this.

[datho7561]

I can confirm that this presents the error in the OpenShift tree view when the tools.json is corrupted. The refactoring looks good as well. Thanks, Andre!

[adietish]

please also review the sonar code smells

There are none left. Sonar keeps reporting 2 false positives which I need to close manually on each push.

[adietish]

@sbouchet corrected things according to your comments. Please re-review.

[sbouchet]

please also review the sonar code smells

There are none left. Sonar keeps reporting 2 false positives which I need to close manually on each push.

seems also that test coverage is missing. need to address that in anotherissue

[adietish]

seems also that test coverage is missing. need to address that in another issue

I agree. Looked through my changes and all what could make sense here imho is to add a test for ApplicationsRootNodeOdo (added/moved in this PR) and file an issue to improve test coverage.

[adietish]

@sbouchet I added a unit test for ApplicationsRootNodeOdo: https://github.com/redhat-developer/intellij-openshift-connector/pull/589/files#diff-148812f161fd7910c4d4c92503a07efee561cc3fc3ce795f7619aa49db4dc0fa
Please re-review. Thanks.

[sbouchet]

LGTM with finals changes.

[sbouchet]

/override ci/prow/e2e-4.11

[openshift-ci]

@sbouchet: Overrode contexts on behalf of sbouchet: ci/prow/e2e-4.11

In response to this:

/override ci/prow/e2e-4.11

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[sonarqubecloud]

SonarCloud Quality Gate failed.   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell

0.0% Coverage
0.2% Duplication

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

[sbouchet]

/check-required-labels

[sbouchet]

/approve

[sbouchet]

/refresh