-
Notifications
You must be signed in to change notification settings - Fork 20
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
adds openshift project key preflight check #630
adds openshift project key preflight check #630
Conversation
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 in general good to me. Just one question: Was the oc client approach easier than using the openshift API or how did you decide that?
src/main/java/org/opendevstack/provision/controller/ProjectApiController.java
Show resolved
Hide resolved
src/main/java/org/opendevstack/provision/services/openshift/OpenshiftService.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opendevstack/provision/services/openshift/OpenshiftService.java
Show resolved
Hide resolved
src/main/java/org/opendevstack/provision/services/openshift/OpenshiftService.java
Show resolved
Hide resolved
@renedupont thanks a lot for help! |
I still believe this is WRONG! ... we tried to decouple OCP as much as possible .. and now we have next to jenkins a direct connection. WHY? @stitakis @michaelsauter what is the usecase for this? |
I'd suggest we revert this PR, and then discuss what problem it is trying to solve, and what approach should be taken to solve the issue. #589 hints at some issues with E2E tests. @stitakis can you explain what those are? If this PR is "just" solving E2E test issues, it is a huge pill to swallow ... |
@clemensutschig @michaelsauter what, what is wrong here? :-) Can you please shed some light on the decoupling on purpose of prov app from OCP. Asking because obviously I wasn't aware of this. From my point of view, I don't see any disavantage on consuming the openshift api to verify if a project key exists (it is not for E2E tests). The ocp api is stable and the dependency in one direction, from ProvApp to ocp api. I'd like understand better the architecture and decisions taken in the past. Happy to discuss on the phone. |
So if it is not for E2E tests, what is it then for? By adding "OpenShift knowledge" to the prov app you are increasing the scope of the prov app. This should be done with great care, as it is already a monster. The prov app does not create OpenShift projects at the moment, but delegates this to Jenkins. The process is completely asynchronous - the prov app never checks the result of the Jenkins Pipeline. I don't think there is anything wrong with triggering the Jenkins Pipeline even if the OpenShift project exists. I'm not sure what its behaviour is right now, but there are just two options: it should either fail because the project already exists (which is OK) or succeed because the project already exists (which is also OK). Allowing to re-run and fill in the gaps even has benefits: Assume one part of the provisioning breaks (e.g. Jira/Bitbucket exists, but OpenShift doesn't), then one can just run the job again. And vice-versa. |
Thanks a lot for the quick feedback! I understand that you are worried and take great care of the ods architecture. I appreciate this a lot and make me feel proud to be part of this great team. :-) Imho in terms of implementation the benefits outweights the cost in this case. I'll explain why... ...this change extends the preflight checks to verify the project key in openshift. It is a plain preflight check. |
but why do you want a pre-flight check on OCP - if the project exists, the jenkins job will start and fail - done ... what more do we need? |
The ProvApp does not have the capability to get/query the job status. It triggers a fire-and-forget request to Jenkins over the webhook proxy. It cannot return job status error to the API consumer. Therefore, this new preflight check is there to verify the project key and give early detailed error to the API consumer. If the project key exists in OCP the ProvApp will return an error to the API caller. |
@stitakis - but what is it needed for?! ... |
@clemensutschig to verify that the project key does not exists before triggering the provisioning... did I understand your last question? I guess no... please give me a quick call, I would be happy to chat with you :-) |
@stitakis - I just dont get why we need a check for this? - the jenkins job will fail - (after successfull start) - and the (internal) project will be saved. so all good. where is the issue here? If it's purely for testing, then I recommend to check that within the test - and delete the project |
is very commons provapp creates projects partially wrong(maybe were able to create jira project and then fails on bitbucket or in OCP), the correct thing here is check everything before start creating. I agree with you that not a good idea that provapp knows about ocp, maybe then webhookproxy(that is the component that provapp is aware) should have the option to ask for if project exists. |
I am confused - there is cleanup etc in place - and by now its pretty resilient by now .. I rather fix this - instead introduce the above., |
i don't trust this cleanup, we disabled it, it destroyed 3 productive projects in the past and caused a lot of problems... |
there was ONE bug - that we fixed - in a cornercase ... (with an existing project) - that should never have happened. can you please elaborate on whats not working? |
I'm not saying it's not working now, is that I'm not feel confident using cleanup feature, is to risky and I think a better approach is to have a preflight check. |
@clemensutschig @segator yes, we need to spend more time in testing the delete project logic. |
What I'm not really getting is:
So if something goes wrong, wouldn't it be nice if I can just re-run the job and it fills in what is missing? I don't think we need cleanup here. I think the best way forward is to have all "actions" triggered by the prov app to be idempotent, meaning I can run the job as often as I want. Each job should check if what it tries to create already exists, and skip. Why do we need cleanup for the "broken provision" use case? |
@stitakis Let's first agree on why we need this query in the first place. I don't get that yet. |
@michaelsauter, it is a preflight check. The why is explained above. Happy to talk with you about it...should I call you? |
there is two usecaes
|
fixes #589