-
Notifications
You must be signed in to change notification settings - Fork 21
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
feat: CI pipeline to build, package, test, and deploy each merged PR #912
Conversation
Fixes: trustification#804 Requires an OPENSHIFT_TOKEN secret that enables privileged access to an OCP instance, e.g. https://api.cluster.trustification.rocks:6443 The following commands were used to generate the token on the OCP host: ``` oc create serviceaccount ${ACCT_NAME} oc create clusterrolebinding ${ACCT_NAME} --clusterrole=cluster-admin --serviceaccount=default:${ACCT_NAME} oc create token ${ACCT_NAME} --duration=$((365*24))h ``` Signed-off-by: Jim Crossley <[email protected]>
Signed-off-by: Jim Crossley <[email protected]>
You can view the results of the workflow on my personal branch: https://github.com/jcrossley3/trustify/actions/runs/11264784849 |
runs-on: ubuntu-22.04 | ||
needs: | ||
- publish | ||
- e2e-test |
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 wanted to point our attention to the fact that we will deploy the code only if the e2e tests passed. I think we should deploy it regardless of the e2e pass or not. Actually, I think the e2e test might or might not fail. Just yesterday you faced an e2e error due to the fact that the the UI of the backend had an outdated version. But I do not hold a strong opinion so I'll leave that to you.
- I thought the idea was to deploy the downstream version of Trustify not the upstream one. That might be just me not getting the requirement correctly though. In any case this is a good milestone!
- I created this issue Do not delete the NS and let the user do it by himself trustify-operator#34 in regards to the fact that you made the trustify-operator scripts to delete the namespace. I think that is a dangerous thing to be set in the trustify-operator scripts and the action of deletion should be done by the user itself (a conscious decision). So Just like you are using
redhat-actions/oc-login@v1
to login in OCP you should delete the Namepace in a command defined in this workflow.
- oc login (
uses: redhat-actions/oc-login@v1
) - oc delete project MY_PROJECT
- Install the oprator
uses: trustification/trustify-operator/.github/actions/install-trustify@main
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 wanted to point our attention to the fact that we will deploy the code only if the e2e tests passed. I think we should deploy it regardless of the e2e pass or not. Actually, I think the e2e test might or might not fail. Just yesterday you faced an e2e error due to the fact that the the UI of the backend had an outdated version. But I do not hold a strong opinion so I'll leave that to you.
Interesting. I think of a CI pipeline as sequential (build, package, test, deploy, etc), failing immediately if any step fails. I like knowing that the thing deployed has passed all our tests, at least the ones known at the time. In the case of the failed ui tests yesterday, we'd only know about that after merging a PR in this repo, which isn't great, but at least we'd know about it eventually, in an automated way.
But if there's a majority opinion either way, I'm happy to abide.
- I thought the idea was to deploy the downstream version of Trustify not the upstream one. That might be just me not getting the requirement correctly though. In any case this is a good milestone!
Easy enough to do -- now that I know how -- but right now both upstream and downstream are identical, except for the konflux config, which isn't buying us anything yet.
Personally, I like for the deployment of the latest thing merged to come out of the repo that merged it.
- I created this issue Do not delete the NS and let the user do it by himself trustify-operator#34 in regards to the fact that you made the trustify-operator scripts to delete the namespace. I think that is a dangerous thing to be set in the trustify-operator scripts and the action of deletion should be done by the user itself (a conscious decision). So Just like you are using
redhat-actions/oc-login@v1
to login in OCP you should delete the Namepace in a command defined in this workflow.
Yeah, I commented on that issue. Deleting the namespace was a hack that gave me a crude, idempotent behavior. And I didn't think it would affect anything you were currently using that script for, because you're always using it in a "virgin" minikube. With a long-lived OCP instance, we don't have the luxury of assuming we can install everything every time.
- oc login (
uses: redhat-actions/oc-login@v1
)- oc delete project MY_PROJECT
- Install the oprator
uses: trustification/trustify-operator/.github/actions/install-trustify@main
To be clear, I don't want to delete the namespace. I want the operator to install a new version of trustify into that namespace. This would significantly reduce the time the app is down. This is the way operators should work, imo, but I need help refactoring that script to achieve that behavior.
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.
To be clear, I don't want to delete the namespace. I want the operator to install a new version of trustify into that namespace. This would significantly reduce the time the app is down. This is the way operators should work, imo, but I need help refactoring that script to achieve that behavior.
Agree. Deleting the NS also means deleting the whole data that was before. I don't want to block your PR, it has been approved, feel free to merge it. But it will be worth investigating how to achieve what you mentioned. Not sure I have too much time to support you in that journey though, but I'll try to find some free time to also suggest some solutions.
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.
wait, I'll mention what I mentioned somewhere in other thread:
This is how Operators work.
- Step 1: The operator is released
- Step 2: you install your operator in OCP
- Step 3: the team releases a new version of the backend (which included the UI) every night.
- Step 4: each night the a new version of the operator is realeased
- Step 5: OLM detects there is a new operator released and automatically updates the container images used in previous release and updates the application to the latest one.
Important to note that the Operator is installed only once in STEP 2.
This is how a real application receives updates when they install an operator. E.g. Today my company installs the Keycloak Operator in my OCP instance and in 1 month the Keycloak teams releases a new version of Keycloak (also the operator). ME as a company should not move a finger to receive updates from Keycloak as my operator, through OLM, should update the Keycloak version as soon as there is a new one.
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.
That magic happens when you install an operator and enable the AUTOMATIC upgrades monitor:
That way, OLM does the magic and you don't need to actually deploy the whole thing after each commit.
But for that to happen a requirement would be:
- The deployment is done nightly and not after each PR
- Nightly a new version is released under a K8s channel
development
and it takes the latest version of container
That is just an idea, feel free to discard it :)
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.
That all sounds good to me, and resonates with what I recall from working with operators in the early days (pre bundles) back when they rarely worked. 😉
- The deployment is done nightly and not after each PR
Why? What does the frequency matter?
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.
Why? What does the frequency matter?
I think yes. But don't take my word for granted, I can be wrong.
For the automatic updates to work 2 things need to happen. A new container version of this repository needs to be created. And second, a new version of the trustify-operator needs to be released at https://github.com/redhat-openshift-ecosystem/community-operators-prod/pulls .
Since we have those 2 conditions, releasing for each commit would mean:
- For each commit here create a new container image of this repository. I THINK THIS IS DONE. No problems
- For each commit in this repository, a gh action in the trustify-operator needs to trigger a new release of the operator https://github.com/trustification/trustify-operator/blob/main/.github/workflows/release.yml (basically to create a new bundle and publish it to a catalog)... doing this for each commit seems expensive and I never saw it at https://github.com/redhat-openshift-ecosystem/community-operators-prod/pulls . We might explore that option though as I might be wrong
They won't have the OPENSHIFT_TOKEN Signed-off-by: Jim Crossley <[email protected]>
Fixes: #804
Requires an
OPENSHIFT_TOKEN
secret that enables privileged access to an OCP instance, e.g. https://api.cluster.trustification.rocks:6443The following commands were used to generate the token on the OCP host:
Upon successful completion of the
latest-release
workflow, the app will be available here:https://trustify-latest-staging.apps.cluster.trustification.rocks/