fabric8-services / fabric8-wit

wit stands for Work Item Tracker

http://devdoc.almighty.io/

Apache License 2.0

45 stars 86 forks source link

Delete workspaces: Don't rely on links in the list workspaces response #2402

Closed jarifibrahim closed 5 years ago

jarifibrahim commented 5 years ago

The response for workspace list returned from che starter does not contain a delete link. This PR removes the check for the delete link in the response payload.

Fixes https://github.com/openshiftio/openshift.io/issues/4559

alien-ike commented 5 years ago

Ike Plugins (test-keeper)

Thank you @jarifibrahim for this contribution!

It appears that no tests have been added or updated in this PR.

Automated tests give us confidence in shipping reliable software. Please add some as part of this change.

If you are an admin or the reviewer of this PR and you are sure that no test is needed then you can use the command /ok-without-tests as a comment to make the status green.

For more information please head over to official documentation. You can find there how to configure the plugin.

jarifibrahim commented 5 years ago

[test]

sbose78 commented 5 years ago

Did you try out any real responses which helped corroborate that deletion was missed because of the if check?

jarifibrahim commented 5 years ago

Did you try out any real responses which helped corroborate that deletion was missed because of the if check?

No. I couldn't try it because che-starter is not exposed publicly and I couldn't run WIT+che-starter+che locally. However, I can be sure there is no Delete link in the response from che-starter because che doesn't return the delete link.

The curl request

curl --header "Authorization: Bearer $tkn" https://che.prod-preview.openshift.io/api/workspace/workspace05p05y6yhser25jr | jq

doesn't have any delete links

The same thing was mentioned in https://github.com/openshiftio/openshift.io/issues/4559#issuecomment-440790477 .

One other important thing is that we have never used the links in the response to delete workspaces. The if check was redundant. https://github.com/fabric8-services/fabric8-wit/blob/f7b1641d891c194976363c4a74d1fe0b575491af/controller/codebase.go#L255-L274

The link variable in the above code snippet is not being used anywhere in the for loop.

sbose78 commented 5 years ago

Got it, @jarifibrahim . We should be good.

sbose78 commented 5 years ago

/ok-without-tests

jarifibrahim commented 5 years ago

[test]

codecov[bot] commented 5 years ago

Codecov Report

Merging #2402 into master will not change coverage. The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #2402   +/-   ##
=======================================
  Coverage   67.75%   67.75%           
=======================================
  Files         183      183           
  Lines       18124    18124           
=======================================
  Hits        12280    12280           
  Misses       4663     4663           
  Partials     1181     1181

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update f7b1641...2819189. Read the comment docs.