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

[KOGITO-8961][KSW-Operator] Split use case and test examples #139

Merged
merged 1 commit into from
Jun 6, 2023

Conversation

desmax74
Copy link
Contributor

See:https://issues.redhat.com/browse/KOGITO-8961

Description of the change:

Motivation for the change:

Checklist

  • Add or Modify a unit test for your change
  • Have you verified that tall the tests are passing?
How to backport a pull request to a different branch?

In order to automatically create a backporting pull request please add one or more labels having the following format backport-<branch-name>, where <branch-name> is the name of the branch where the pull request must be backported to (e.g., backport-7.67.x to backport the original PR to the 7.67.x branch).

NOTE: backporting is an action aiming to move a change (usually a commit) from a branch (usually the main one) to another one, which is generally referring to a still maintained release branch. Keeping it simple: it is about to move a specific change or a set of them from one branch to another.

Once the original pull request is successfully merged, the automated action will create one backporting pull request per each label (with the previous format) that has been added.

If something goes wrong, the author will be notified and at this point a manual backporting is needed.

NOTE: this automated backporting is triggered whenever a pull request on main branch is labeled or closed, but both conditions must be satisfied to get the new PR created.

@desmax74 desmax74 marked this pull request as draft May 30, 2023 11:55
@desmax74 desmax74 marked this pull request as ready for review May 30, 2023 12:40
@desmax74 desmax74 marked this pull request as draft May 30, 2023 15:03
@desmax74 desmax74 marked this pull request as ready for review May 30, 2023 16:24
@desmax74 desmax74 force-pushed the kogito-8961 branch 2 times, most recently from b88f893 to 84489e3 Compare May 30, 2023 16:53
@desmax74 desmax74 requested a review from spolti May 30, 2023 17:24
@@ -31,11 +31,12 @@ import (

func TestKogitoServerlessBuildController(t *testing.T) {
namespace := t.Name()
ksw := test.GetKogitoServerlessWorkflow("../config/samples/"+test.KogitoServerlessWorkflowSampleYamlCR, namespace)
ksw := test.GetBaseServerlessWorkflowNoPackage(namespace)
Copy link
Member

Choose a reason for hiding this comment

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

What package means?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have some test that needs as a path "../config/samples/" to load the yaml's files, others "../../config/samples/" because they are inside a package

Copy link
Member

Choose a reason for hiding this comment

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

I think we should use a smarter way of doing that. Maybe we can infer the path we are and compose it dynamically. If you think it is worth a shot, it would be a great improvement. I wouldn't do it if it takes too much time.

Copy link
Contributor Author

@desmax74 desmax74 Jun 1, 2023

Choose a reason for hiding this comment

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

@ricardozanini is it ok for your this https://github.com/kiegroup/kogito-serverless-operator/pull/139/files#diff-8affcd5838f425d5db07c12738684112559a18621ec539e9b33986abe804db7fR124-R132 in conjunction with the runtime.Caller ? No other ideas to decide the path, but I'm open if you have better

Copy link
Member

Choose a reason for hiding this comment

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

test/e2e/workflow_test.go Outdated Show resolved Hide resolved
test/yaml.go Outdated Show resolved Hide resolved
test/yaml.go Outdated Show resolved Hide resolved
@desmax74 desmax74 force-pushed the kogito-8961 branch 6 times, most recently from 0e2d838 to 91f326f Compare May 31, 2023 12:45
@@ -1,10 +0,0 @@
apiVersion: sw.kogito.kie.org/v1alpha08
Copy link
Member

Choose a reason for hiding this comment

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

didn't we discuss the we wouldn't change the config/samples?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have agreed the proposal of @davidesalerno to use the needed yaml files and create the structs with the few fields needed in the tests instead to have one yaml file for each test, this file is replaced by the platform plus the field with the base image

Copy link
Member

@ricardozanini ricardozanini Jun 1, 2023

Choose a reason for hiding this comment

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

The config/samples directory is used by the OLM/Operator SDK to compose the CSV. Meaning that users will see these examples on the OpenShift console.

Bear that in mind and keep only the simplest ones. I think this is the proposal, right?

Copy link
Member

Choose a reason for hiding this comment

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

So we should keep here only the ones that will be in the CSV, one very simple example per custom resource.

Copy link
Member

Choose a reason for hiding this comment

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

For example, see here: https://operatorhub.io/operator/nexus-operator-m88i
Click on "View Example".

That's the purpose of this directory.

Copy link
Contributor Author

@desmax74 desmax74 Jun 1, 2023

Choose a reason for hiding this comment

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

@ricardozanini Yep the file used by kustomization
https://github.com/kiegroup/kogito-serverless-operator/blob/main/config/samples/kustomization.yaml
are only:

sw.kogito_v1alpha08_kogitoserverlessworkflow.yaml
sw.kogito_v1alpha08_kogitoserverlessplatform.yaml 

and in the folder in the PR we leave only these two and sw.kogito_v1alpha08_kogitoserverlessworkflow_devmodeWithConfigMapAndExternalResource.yaml with a complete example, but we can move it in the docs folder

func GetPathForSamples(path string) string {
operatorPath := strings.Split(path, "kogito-serverless-operator")[1]
packages := strings.Count(operatorPath, "/")
if strings.Contains(operatorPath, "/test/") || packages == 3 {
Copy link
Member

Choose a reason for hiding this comment

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

Not required, we can do it later. But maybe the filepath package might help here? https://gobyexample.com/file-paths

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We do later, is a func used only in test

Copy link
Member

@ricardozanini ricardozanini left a comment

Choose a reason for hiding this comment

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

Sorry to say this just now, but reading your comment here https://github.com/kiegroup/kogito-serverless-operator/pull/143#discussion_r1213619180 made me realize that we should move the resources to testdata instead of tests, so it won't be considered a go package by go tooling.

  1. Keep in config/samples the excerpt for the CSV as we discussed earlier
  2. Use testdata instead of tests for the unit tests within the project

@desmax74 desmax74 force-pushed the kogito-8961 branch 2 times, most recently from 307dc37 to 632740a Compare June 5, 2023 08:51
@desmax74 desmax74 force-pushed the kogito-8961 branch 2 times, most recently from cbf8e6e to 18ce8a8 Compare June 5, 2023 20:41
@desmax74 desmax74 marked this pull request as draft June 5, 2023 20:45
@desmax74 desmax74 marked this pull request as ready for review June 5, 2023 20:48
@desmax74 desmax74 force-pushed the kogito-8961 branch 4 times, most recently from 2b1b589 to 0c2053c Compare June 6, 2023 09:23
Signed-off-by: desmax74 <mdessi@redhat.com>
@desmax74
Copy link
Contributor Author

desmax74 commented Jun 6, 2023

@davidesalerno @radtriste

@ricardozanini ricardozanini merged commit 5b0bdba into apache:main Jun 6, 2023
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants