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

Allow Openshift deploy_che.sh script to detect if project deletion is completed and new project successfully created. #8159

Merged
merged 5 commits into from
Jan 17, 2018

Conversation

JamesDrummond
Copy link
Contributor

What does this PR do?

Allow Openshift deploy_che.sh script to detect if project deletion is completed and new project successfully created.

What issues does this PR fix or reference?

N/A

Release Notes

Allow Openshift deploy_che.sh script to detect if project deletion is completed and new project successfully created.

Signed-off-by: James Drummond james@devcomb.com

… completed and new project successfully created.

Signed-off-by: James Drummond <james@devcomb.com>
@JamesDrummond JamesDrummond added kind/bug Outline of a bug - must adhere to the bug report template. kind/enhancement A feature request - must adhere to the feature request template. labels Jan 3, 2018
@JamesDrummond JamesDrummond added this to the 6.0.0-M5 milestone Jan 3, 2018
@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 Jan 3, 2018
@JamesDrummond JamesDrummond self-assigned this Jan 4, 2018
@codenvy-ci
Copy link

Can one of the admins verify this patch?

@benoitf benoitf removed their request for review January 11, 2018 10:10
@JamesDrummond
Copy link
Contributor Author

@riuvshin @benoitf @sleshchenko Please review.

@@ -282,13 +282,39 @@ if ! oc get project "${CHE_OPENSHIFT_PROJECT}" &> /dev/null; then
if [ "${COMMAND}" == "cleanup" ] || [ "${COMMAND}" == "rollupdate" ]; then echo "**ERROR** project doesn't exist. Aborting"; exit 1; fi
if [ "${OPENSHIFT_FLAVOR}" == "osio" ]; then echo "**ERROR** project doesn't exist on OSIO. Aborting"; exit 1; fi

echo -n "Project does not exist...creating it..."
oc new-project "${CHE_OPENSHIFT_PROJECT}" &> /dev/null
# OpenShift will not get project but project still exists for for a period after being deleted.
Copy link
Member

Choose a reason for hiding this comment

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

typo double for

do
{ # try

oc new-project "${CHE_OPENSHIFT_PROJECT}" &> /dev/null && \
Copy link
Member

Choose a reason for hiding this comment

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

It can fail if another user has a project with the same name. So infinitive loop will be in this case. Please consider adding timeout for waiting.

Copy link
Contributor Author

@JamesDrummond JamesDrummond Jan 15, 2018

Choose a reason for hiding this comment

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

@sleshchenko I don't think you can have two projects with the same name in OpenShift can you? This is one of the reason I created PR #8154. I can add a timeout though anyways.

Copy link
Member

Choose a reason for hiding this comment

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

@JamesDrummond You can't. So script expects that error is temporary (like a project is marked to remove and removing is in progress) and it will disappear when a project will be deleted.
But error can be permanent - like another user has a project with the same name. Then infinitive loop will be here.
P.S. Let me know if my explanation is not clear enough. I'll try to reword it.

Copy link
Contributor Author

@JamesDrummond JamesDrummond Jan 15, 2018

Choose a reason for hiding this comment

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

@sleshchenko In OpenShift I don't think it is possible to have two projects with the same name. Even seperate users can't use a project name if it's already taken. I actually remember when I used osio this was even an issue. Took some screen captures below where james@devcomb.com has eclipse-che project and developer user cannot create a project with same name.

image

image

image

If another user just happens to create a project with the same name right after you delete yours, the user can kill the script manually. This is not a likely event. If the project has issues being deleted then this is really another problem that is not likely to happen. I use this script often and rarely have any issues.

Copy link
Contributor

@riuvshin riuvshin Jan 15, 2018

Choose a reason for hiding this comment

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

everybody agrees that you can't have 2 projects with the same name, but what @sleshchenko meant is that if this error will happen script will go into endless loop which is no OK and there is should be a timer or some kind of interruptor.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@riuvshin Sorry didn't understand. Will add timer. Thanks.

@JamesDrummond
Copy link
Contributor Author

JamesDrummond commented Jan 15, 2018

@sleshchenko @riuvshin Added timeout and tested. Please review again.


echo -n "[CHE] Switching to \"${CHE_OPENSHIFT_PROJECT}\"..."
oc project "${CHE_OPENSHIFT_PROJECT}" &> /dev/null
WAIT_TO_SWITCH_TO_PROJECT=true
Copy link
Member

@sleshchenko sleshchenko Jan 16, 2018

Choose a reason for hiding this comment

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

Do we really need this waiting? As far as I see a current project is automatically switched to newly created after oc new-project. When there can be a delay between project creation (oc new-project) and ability to switch to it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sleshchenko I think the intent was to allow switch if project already existed but I cannot see the reason to do this anymore. Removed.

@JamesDrummond
Copy link
Contributor Author

@sleshchenko Please review.

@JamesDrummond JamesDrummond merged commit 3d1e4c8 into master Jan 17, 2018
@JamesDrummond JamesDrummond deleted the openshift_init_patch5 branch January 17, 2018 16:12
@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 Jan 17, 2018
@sleshchenko
Copy link
Member

@JamesDrummond Have you tested rollupdate command after your changes?

It doesn't work for me.

screenshot_20180118_101016

Looks like this else section brokes rollupdate command https://github.com/eclipse/che/pull/8159/files#diff-c656525c29bc2779734318b21b1b983dR313

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. kind/enhancement A feature request - must adhere to the feature request template.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants