-
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
Use the Makefile to build and push in GH actions #313
Conversation
8d7faed
to
094a9ea
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.
1 little clean up and it's all good
BTW, the PR title does not reflects what it actually brings (uniformisation of the image and manifests generation and removal of the auto PR)
Sorry for the misleading pr title, I also noticed that, and changed it.
I'll send another PR with the actual disabling of the send and leave this
PR as a pure refactoring one.
I'll remove the DEV related args as well, and for future use I think that
we can expose a second arg to extend the arg if we want.
something like:
ARG EXTRA_QUARKUS_EXTENSIONS
ARG QUARKUS_EXTENSIONS="defaults"
ARG QUARKUS_EXTENSIONS="${QUARKUS_EXTENSIONS}${EXTRA_QUARKUS_EXTENSIONS}"
No one yet used something like that or needed that, but it is an option if
we want to add it and keep the nice defaults in the dockerfile. Anyhow, not
in this PR
…On Mon, 15 Jul 2024 at 10:48, gabriel-farache ***@***.***> wrote:
***@***.**** requested changes on this pull request.
1 little clean up and it's all good
BTW, the PR title does not reflects what it actually brings
(uniformisation of the image and manifests generation and removal of the
auto PR)
------------------------------
In Makefile
<#313 (comment)>
:
> @@ -90,13 +90,9 @@ ENABLE_PERSISTENCE ?= false
# extra extensions needed for persistence at build time.
# The extentions listed below are included in the cache in image quay.io/kiegroup/kogito-swf-builder:9.99.1.CR1 or available from maven central repository
-QUARKUS_EXTENSIONS=org.kie.kogito:kogito-addons-quarkus-jobs-knative-eventing:9.99.1.redhat-00003,org.kie.kogito:kogito-addons-quarkus-persistence-jdbc:9.99.1.redhat-00003,org.kie.kogito:kogito-addons-persistence-jdbc:9.99.1.redhat-00003,io.quarkus:quarkus-jdbc-postgresql:3.2.9.Final,io.quarkus:quarkus-agroal:3.2.9.Final,org.kie:kie-addons-quarkus-monitoring-prometheus:999-SNAPSHOT,org.kie:kie-addons-quarkus-monitoring-sonataflow:999-SNAPSHOT
QUARKUS_DEV_EXTENSIONS=""
if you remove the prod value, please also remove the dev one
—
Reply to this email directly, view it on GitHub
<#313 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABGBYHDKNGAJWHVIMNXNHYDZMN5DRAVCNFSM6AAAAABK3INRPCVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDCNZXGA2TCOJRHE>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
9d6fe98
to
ad1d963
Compare
f57f747
to
a607d44
Compare
Make the Dockerfile the source of truth for the build, so no arguments other than worklow identifier is needed to build. The assumption is that all our workflow needs the same extensions to operate properly in the cluster. Signed-off-by: Roy Golan <[email protected]>
With this change we make sure all the build and push goes through the make. A failure in CI would likely indicate that developers fail to build images, and vice versa, given CI and local users use the same buidlah version - currently github runner ubuntu 22 uses buildah 1.23.1 which is bit dated. The remaining bits that don't use the makefile is the generation of the PR to the CD repo. The current github actions have specific code to push stuff for workflows in helm format, and the makefile invokes a script that does it for kustomize format. We should unit the two behaviours to the scripts can handle both, or just make all workflows use a single format, and then use that from the action. Signed-off-by: Roy Golan <[email protected]>
Signed-off-by: Roy Golan <[email protected]>
make gen-manifests would bail out without setting the image when it was set to false. To fix it that section is in its own condition, and the rest of the script can continue if need. Also the default is now true. Another addition to the Makefile is that we now don't need to specify the WORKFLOW_ID argument but instead can call it like a target: make move2kube gen-manifests Signed-off-by: Roy Golan <[email protected]>
Use an argument to represent the workflow folder, and use it where expected, for example to generate the container image name and so on. This is now separate from the workflow_id we extract form the yaml spec to operate on the manifest files (stuff like yq replacement we perform) Signed-off-by: Roy Golan <[email protected]>
Support the discovery of all both yaml and yml workflow files Signed-off-by: Roy Golan <[email protected]>
The Makefile default is to use a short 8 chars of the revision, while the github action is relying on the full github version using the {{ github.sha }} context variable, which is available every where. We could potentially change the short sha in the makefile, but it is convenient for developers, hence will leave this untouched for now. Signed-off-by: Roy Golan <[email protected]>
This will help use perform gen-manifests in and easy way Signed-off-by: Roy Golan <[email protected]>
79ed6f3
to
4eb1b59
Compare
Signed-off-by: Roy Golan <[email protected]>
@rgolangh the error is about incorrect request to invoke the MTA that now requires the recipient, which is an array that its elements should be in the format of so this line https://github.com/parodos-dev/serverless-workflows/blob/main/e2e/mta.sh#L31
should be changed to:
that should be done for all workflows that requires notifications (all but greeting) |
Signed-off-by: Roy Golan <[email protected]>
Signed-off-by: Roy Golan <[email protected]>
the state of this PR is that I'm working to get the upstream notification plugins working, because they don't at the moment. |
Now, we may need to startup a simple webserver (i.e: https://httpbin.org/) that does nothing and configure the notification URL to point to that dumb webserver |
375c003
to
ccc5bdb
Compare
Right now the upstream notification service exported at https://github.com/redhat-developer/rhdh-plugin-export-backstage-backstage/releases is not working due to some compatibility issues with upstream RHDH backstage version. Till we solve this problem the e2e test will use a fake notifications pod that would log all the requests to the container log. We can examine the container logs and make sure the notifications were sent in the right format, for now. Signed-off-by: Roy Golan <[email protected]>
Signed-off-by: Roy Golan <[email protected]>
Use the Makefile to build and push in GH actions
With this change we make sure all the build and push goes through the
make. A failure in CI would likely indicate that developers fail to
build images, and vice versa, given CI and local users use the same
buidlah version - currently github runner ubuntu 22 uses buildah 1.23.1
which is bit dated.
The remaining bits that don't use the makefile is the generation of the
PR to the CD repo. The current github actions have specific code to push
stuff for workflows in helm format, and the makefile invokes a script
that does it for kustomize format. We should unit the two behaviours to
the scripts can handle both, or just make all workflows use a single
format, and then use that from the action.
There should be another PR sent to the orchestrator-helm-chart to cleanup all the tekton pipeline custom build args that with that PR will be
defaults in the Dockerfile - rhdhorchestrator/orchestrator-helm-chart#221