Skip to content
This repository has been archived by the owner on Jul 23, 2024. It is now read-only.

separate workflow-examples from workflow-service #470

Merged
merged 1 commit into from
Aug 10, 2023

Conversation

ydayagi
Copy link
Contributor

@ydayagi ydayagi commented Jul 6, 2023

build workflow-service image without workflow-examples
build an image with workflow-examples for integration tests

FLPATH-510 https://issues.redhat.com/browse/FLPATH-510

@openshift-ci openshift-ci bot requested review from nirarg and pkliczewski July 6, 2023 00:33
.github/workflows/test.yml Show resolved Hide resolved
hack/manifests/base/deployment.yaml Outdated Show resolved Hide resolved
hack/manifests/testing/copy-to-pvc.yaml Outdated Show resolved Hide resolved
.github/workflows/test.yml Outdated Show resolved Hide resolved
Copy link
Collaborator

@eloycoto eloycoto left a comment

Choose a reason for hiding this comment

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

The code looks good, but I do not think that it's the best way to present this to the users. At the end, this is not composable at all.

I would like to be the jars part of the Dockerfile, so the user can push the Jar inside the Dockerfile and keep track of IDS and versions.

I cannot see how a rollback in a version will work here or how can a user can register 10 different jars to compose user flows.

Not sure if this is the right direction.

@ydayagi
Copy link
Contributor Author

ydayagi commented Jul 11, 2023

The code looks good, but I do not think that it's the best way to present this to the users. At the end, this is not composable at all.

I would like to be the jars part of the Dockerfile, so the user can push the Jar inside the Dockerfile and keep track of IDS and versions.

I cannot see how a rollback in a version will work here or how can a user can register 10 different jars to compose user flows.

Not sure if this is the right direction.

@eloycoto @masayag @pkliczewski
Eloy please suggest another solution.
Regarding, rollback. I don't see how putting the JARs in the Dockerfile helps. As long as u have two separate entities/deliverables: user JARs and our JARs, you will have a problem.
Supporting multiple JARs is possible. it is just a decision.

@@ -14,6 +14,7 @@ patches:

images:
- name: quay.io/parodos-dev/workflow-service:main
newName: quay.io/parodos-dev/integration-service
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a need to create this repository on quay?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it is only a tag. it is never pushed to quay

Copy link
Collaborator

Choose a reason for hiding this comment

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

should main added here, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@eloycoto what do u mean?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Should be like this:

- name: quay.io/parodos-dev/workflow-service:main
  newTag: test
- newName: quay.io/parodos-dev/integration-service:main
  newTag: test
- name: quay.io/parodos-dev/notification-service:main
  newTag: test

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no. this is how it should be. and it works

@ydayagi ydayagi force-pushed the flpath510 branch 5 times, most recently from 4521f90 to dcf9c85 Compare July 19, 2023 16:00
@masayag
Copy link
Collaborator

masayag commented Jul 20, 2023

/lgtm

Copy link
Collaborator

@eloycoto eloycoto left a comment

Choose a reason for hiding this comment

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

I'm not sure if the push-image thing is implemented, I do not think so.

At the other hand, maybe the update on hack/manifest is needed, to use the image in all manifest, if not only the workflow-service without workflows examples in there.

I would check all .github/actions/ and hack/manifest/

@ydayagi
Copy link
Contributor Author

ydayagi commented Aug 2, 2023

I'm not sure if the push-image thing is implemented, I do not think so.

At the other hand, maybe the update on hack/manifest is needed, to use the image in all manifest, if not only the workflow-service without workflows examples in there.

I would check all .github/actions/ and hack/manifest/

@masayag @eloycoto @pkliczewski I don't understand what is needed. the required changes for building an image for integration tests were made. it does not effect release.

build workflow-service image without workflow-examples
provide workflow-examples (or user's JAR) via a PVC
for integration tests create a PVC in kind and use a pod to mount it and copy the examples JAR to the PVC.

FLPATH-510 https://issues.redhat.com/browse/FLPATH-510

Signed-off-by: Yaron Dayagi <[email protected]>
@masayag
Copy link
Collaborator

masayag commented Aug 10, 2023

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Aug 10, 2023
@pkliczewski
Copy link
Collaborator

@eloycoto what do you think?

Copy link
Collaborator

@eloycoto eloycoto left a comment

Choose a reason for hiding this comment

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

LGTM, but:

@gabriel-farache, maybe we should update the image in the operator default one?
@ydayagi did you create the image in the quay.io?

I'll notify users to use examples-service instead of workflow-service. cc @RichardW98

@gabriel-farache
Copy link
Contributor

@eloycoto

@gabriel-farache, maybe we should update the image in the operator default one?

When we release the new version of workflow-service, the stable image will not have the examples, as there are in a separate image now; so do you mean we should add the example image to the operator?
BTW how that split will reflect in the manifest we release?

@pkliczewski
Copy link
Collaborator

@gabriel-farache @eloycoto what do you suggest to do to finalize this PR?

@eloycoto
Copy link
Collaborator

Quay container created:
https://quay.io/repository/parodos-dev/examples-service

@eloycoto
Copy link
Collaborator

/lgtm
/approve

@openshift-ci
Copy link

openshift-ci bot commented Aug 10, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: eloycoto

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-robot openshift-merge-robot merged commit 33c73f6 into parodos-dev:main Aug 10, 2023
5 checks passed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants