-
Notifications
You must be signed in to change notification settings - Fork 54
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
Swtich from 'odo' to 'oc' for create/set/get active/delete project and list ptojects #3826
Swtich from 'odo' to 'oc' for create/set/get active/delete project and list ptojects #3826
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #3826 +/- ##
==========================================
+ Coverage 32.41% 32.47% +0.06%
==========================================
Files 85 85
Lines 6396 6418 +22
Branches 1321 1333 +12
==========================================
+ Hits 2073 2084 +11
- Misses 4323 4334 +11 ☔ View full report in Codecov by Sentry. |
7858d98
to
bebb7d9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two small changes, but otherwise seems to working very well and the code looks good. Thanks, Victor!
src/oc/ocWrapper.ts
Outdated
new CommandText('oc', `delete ${obj} ${projectName}`) | ||
) | ||
.then((result) => result.stdout) | ||
.catch((error) => error.message); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will prevent the error from being thrown. This means that in order to tell if creating the project succeeds or fails, you will need to parse the string that's returned. I think it's better to avoid catching the error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@datho7561 Hm. Actually it's not correct. Previously we were using -f
parameter when running odo
commands which was preventing it from throwing any exceptions (f.i. when deleting non-existing projects).
Catching errors when an oc
command is used makes it work asodo xxx -f
did before, otherwise we'll have to surround every call to Oc.Instance.deleteProject()
with a try-catch or errors in some other way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that it will probably change some behaviour, but I think it should be okay. We should avoid presenting the option to delete a project that doesn't exist in the UI. Even if you attempted to, it would still display the error as a popup notification. (To confirm this, try deleting the default
project on a kind
cluster)
…d list ptojects Issue: redhat-developer#3662 Signed-off-by: Victor Rubezhny <vrubezhny@redhat.com>
bebb7d9
to
48612ba
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Thanks, Victor!
Issue: #3662