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.

Sign up for GitHub

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

Jump to bottom

Use the Makefile to build and push in GH actions #313

Merged
merged 13 commits into from
Jul 22, 2024
Merged

Use the Makefile to build and push in GH actions #313

merged 13 commits into from
Jul 22, 2024

Conversation

rgolangh
Copy link
Collaborator

@rgolangh rgolangh commented Jul 14, 2024 •
edited

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 - parodos-dev/orchestrator-helm-chart#221

@rgolangh rgolangh mentioned this pull request Jul 14, 2024
Merged
@rgolangh rgolangh force-pushed the ci-stop-auto-pr-sending branch 2 times, most recently from 8d7faed to 094a9ea Compare July 14, 2024 16:18
gabriel-farache
gabriel-farache requested changes Jul 15, 2024
View reviewed changes
Copy link
Collaborator

@gabriel-farache gabriel-farache left a 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)

Makefile Outdated Show resolved Hide resolved
@rgolangh rgolangh changed the title ci stop auto pr sending Use the Makefile to build and push in GH actions Jul 15, 2024
@rgolangh
Copy link
Collaborator Author

rgolangh commented Jul 15, 2024 via email

masayag
masayag requested changes Jul 15, 2024
View reviewed changes
.github/workflows/main.yml Outdated Show resolved Hide resolved
.github/workflows/main.yml Show resolved Hide resolved
@rgolangh rgolangh mentioned this pull request Jul 15, 2024
Closed
@masayag
Copy link
Collaborator

masayag commented Jul 16, 2024

@rgolangh #317 was merged,
pls update this PR and let's see if that solves (some) of the issues.
The missing piece will probably be generating the token for the notifications.

@rgolangh
Move all default into the Dockerfile
43062a4 
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 <rgolan@redhat.com>
@rgolangh
Use the Makefile to build and push in GH actions
ab2d0e8 
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 <rgolan@redhat.com>
@rgolangh
e2e: cleanup obsolete comments
e38d943 
Signed-off-by: Roy Golan <rgolan@redhat.com>
@rgolangh
Don't bail out from gen-manifests early
89eed78 
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 <rgolan@redhat.com>
@rgolangh
gen-manifests: use the workflow folder name, not the workflow_id
8f72453 
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 <rgolan@redhat.com>
@rgolangh
gen-manifests: support *.sw.yaml or *.sw.yml
262edc1 
Support the discovery of all both yaml and yml workflow files

Signed-off-by: Roy Golan <rgolan@redhat.com>
@rgolangh
Pass the full github sha to the make targets in github actions
975f1ef 
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 <rgolan@redhat.com>
@rgolangh
Add kn and kubectl binaries to the helper image 'ubi9-pipeline'
024928d 
This will help use perform gen-manifests in and easy way

Signed-off-by: Roy Golan <rgolan@redhat.com>
@rgolangh rgolangh force-pushed the ci-stop-auto-pr-sending branch 2 times, most recently from 79ed6f3 to 4eb1b59 Compare July 16, 2024 14:57
@rgolangh
e2e: Set the notification bearer token for workflows under test
4eb1b59 
Signed-off-by: Roy Golan <rgolan@redhat.com>
@masayag
Copy link
Collaborator

masayag commented Jul 16, 2024

@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 user:default/guest or user:development/guest (depending on the available namespaces).

so this line https://github.com/parodos-dev/serverless-workflows/blob/main/e2e/mta.sh#L31

out=$(curl -XPOST -H "Content-Type: application/json"  http://localhost:9080/api/orchestrator/workflows/MTAAnalysis/execute -d '{"repositoryURL": "https://github.com/spring-projects/spring-petclinic"}')

should be changed to:

out=$(curl -XPOST -H "Content-Type: application/json"  http://localhost:9080/api/orchestrator/workflows/MTAAnalysis/execute -d '{"repositoryURL": "https://github.com/spring-projects/spring-petclinic", "recipients": ["user:default/guest"]}')

that should be done for all workflows that requires notifications (all but greeting)

@rgolangh
e2e: Use the supported RHDH-backstage helm chart
772c1e5 
Signed-off-by: Roy Golan <rgolan@redhat.com>
@rgolangh
e2e: Send 'recipients' mandatory notification argument
76b0799 
Signed-off-by: Roy Golan <rgolan@redhat.com>
@rgolangh
Copy link
Collaborator Author

the state of this PR is that I'm working to get the upstream notification plugins working, because they don't at the moment.
We may need to think on how to mock/mimic/disable the notifications in some way, so e2e would pass,so we can merge this work because this PR will obviously get stale in time and we would loose the other

@gabriel-farache
Copy link
Collaborator

We may need to think on how to mock/mimic/disable the notifications in some way
When the notification used to be optional, it was easy, we simply had to not provide the recipients.

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

@rgolangh rgolangh force-pushed the ci-stop-auto-pr-sending branch 5 times, most recently from 375c003 to ccc5bdb Compare July 18, 2024 19:46
@rgolangh
e2e: Use fake notifications service as a workaround
ccc5bdb 
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 <rgolan@redhat.com>
@rgolangh
e2e: Give more time to sonata dataindex to properly start
182051d 
Signed-off-by: Roy Golan <rgolan@redhat.com>
@rgolangh rgolangh mentioned this pull request Jul 22, 2024
Closed
@masayag masayag merged commit 5f784c3 into main Jul 22, 2024
10 of 16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@masayag masayag masayag approved these changes

@gabriel-farache gabriel-farache gabriel-farache approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@rgolangh @masayag @gabriel-farache