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

Use notifications specs from github repo #55

Merged
merged 2 commits into from
Jan 30, 2024
Merged

Use notifications specs from github repo #55

merged 2 commits into from
Jan 30, 2024

Conversation

gabriel-farache
Copy link
Collaborator

Use notifications specs from github repo to avoid duplicate files in each workflow specs

@gabriel-farache
Copy link
Collaborator Author

@rgolangh in the MTA props, I do not see where you set the URL for the notification, is it missing?

@gabriel-farache
Copy link
Collaborator Author

/hold Do not merge until notification plugin is made a dynamic plugin: the PR uses the specs from release 1.0.3 (https://github.com/janus-idp/backstage-plugins/releases/tag/%40janus-idp%2Fplugin-notifications-backend%401.0.3)
Also maybe waits for https://issues.redhat.com/browse/FLPATH-878 as it fixes bugs with arrays and the specs contains arrays (for actions and targetUsers for instance)

Copy link
Collaborator

@dmartinol dmartinol left a comment

Choose a reason for hiding this comment

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

Thank you!

@masayag
Copy link
Collaborator

masayag commented Jan 25, 2024

@gabriel-farache pls rebase. (or wait for few more days and then rebase :-) )

@rgolangh
Copy link
Collaborator

@rgolangh in the MTA props, I do not see where you set the URL for the notification, is it missing?

I think there's was a url in the openapi spec

@rgolangh
Copy link
Collaborator

So I still think we need a property for MTA for notification endpoint , because the default in the openapi spec
is localhost:7007
@gabriel-farache Can you please add quarkus.rest-client.notifications.url=${BACKSTAGE_NOTIFICATIONS_URL:http://orchestrator-backstage.orchestrator/api/notifications/} to the application props of mta?

@rgolangh
Copy link
Collaborator

we would need to add , for e2e , an env variable to point at the e2e endpoint, similar to
https://github.com/parodos-dev/serverless-workflows/blob/37054d2ff27d83fffa600475e77eca7f11c41e27/.github/workflows/mta-e2e.yaml#L103

@gabriel-farache
Copy link
Collaborator Author

So I still think we need a property for MTA for notification endpoint , because the default in the openapi spec
is localhost:7007
@gabriel-farache Can you please add quarkus.rest-client.notifications.url=${BACKSTAGE_NOTIFICATIONS_URL:http://orchestrator-backstage.orchestrator/api/notifications/} to the application props of mta?

we would need to add , for e2e , an env variable to point at the e2e endpoint, similar to

@rgolangh done :)

@gabriel-farache
Copy link
Collaborator Author

/unhold

@gabriel-farache
Copy link
Collaborator Author

gabriel-farache commented Jan 30, 2024

apache/incubator-kie-kogito-runtimes#3377 is merged so sonataflow should have the openapi generator version that includes the fix in tomorrow's nightly

And rhdhorchestrator/orchestrator-helm-chart#45 is including the notification plugin as a dynamic one in the charts

@masayag
Copy link
Collaborator

masayag commented Jan 30, 2024

@gabriel-farache some of the changes were pushed in a previously merged PR. Please rebase.

@gabriel-farache
Copy link
Collaborator Author

@gabriel-farache some of the changes were pushed in a previously merged PR. Please rebase.

@masayag done

@masayag masayag merged commit 0c7c9b0 into main Jan 30, 2024
3 checks passed
@gabriel-farache gabriel-farache deleted the shared_specs branch January 31, 2024 10:20
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.

4 participants