keboola / app-orchestrator-trigger

MIT License
0 stars 1 forks source link

Use Trigger orchestration API call #8

Open davidesner opened 3 years ago

davidesner commented 3 years ago

I changed the API call so it uses the triggerOrchestration - sends nontifications properly and doesn't execute disabled orchestrations. To bypass this historically I had to create the esnerda.app-orchestrator-trigger-mod app that is now being used by a lot of clients unfortunately. I think we should publish this one.

davidesner commented 3 years ago

@ErikZigo @webrouse We are also using the esnerda.app-orchestrator-trigger-mod in the Academy sessions and the app is useful for lot of clients. It's confusing to people that it is not public. I think it should be public and there should be just a single one. What do you think?

BTW I see there's lot of changes of function parameter datatypes in the 1.2.3 keboola/orchestrator-php-client so the tests are failing, I will leave that to you

michaljurecko commented 3 years ago

I don't know exactly what this is about and I see that the tests don't pass, I have something to do with that? @ErikZigo

tomasfejfar commented 3 years ago

For some reason I'm receiving updated for this, so I might as well react. Tests fail because ENV variables are not passed to PRs (for security). You need to create new branch on top of the PR and continue there IMHO.

Regarding the PR itself - it seems reasonable for the behavior to be configurable. There are many scenarios where I want the trigger to respect whether the orchestration is disabled or not IMHO. But the other way around is also important - therefore both should be possible.

davidesner commented 3 years ago

Regarding the PR itself - it seems reasonable for the behavior to be configurable. There are many scenarios where I want the trigger to respect whether the orchestration is disabled or not IMHO. But the other way around is also important - therefore both should be possible.

Do you have some example scenario where user would want to trigger a disabled orchestration? I can't think of any really. But I get that if someone is depending on this atm (possibly unwillingly) this change would break such configurations.

tomasfejfar commented 3 years ago

Exactly. BC is important, or at least planned deprecation needs to happen.

Maybe you have an orchestration planned daily, that can be disabled at times (for example when you have company-wide vacation), but you want to run it nevertheless when manually triggered once a month.

davidesner commented 3 years ago

I still don't get it why would I disable orchestration and wanted to trigger it from another orchestration using the app. This would be completely intransparent, someone disables transformation, because it's not supposed to be scheduled nor executed.Let's say something breaks and you don't want it to execute until its fixed, so you disable it and the natural thing is to expect, that no external process would be able to run it. In the UI when you disable orchestration it is not possible to trigger it even manually (via UI). I definitely wouldn't encourage anyone to build the pipeline depending on such shady hacks. Not to mention that this behaviour is not documented anywhere so once users discovered this, it was reported as a bug.

I can push this via Ideas as well. I just wanted it to bring it to attention. I don't think it's a good thing that we teach people to use the unpublished fork of this app, some users still use this original one and it's all confusing. And since we push the "multiproject-architecture" methodology everywhere this app is and will be widely used - unless the functionality is somehow implemented in the product.

On Tue, Apr 27, 2021 at 11:21 AM Tomáš Fejfar @.***> wrote:

Exactly. BC is important, or at least planned deprecation needs to happen.

Maybe you have an orchestration planned daily, that can be disabled at times (for example when you have company-wide vacation), but you want to run it nevertheless when manually triggered once a month.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/keboola/app-orchestrator-trigger/pull/8#issuecomment-827457207, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADXN23GRHOHNJTON6AXHNVDTKZ63NANCNFSM43MITVKA .

--

David Esner| Data Engineer | Consultant | Prague [image: https://www.keboola.com/] https://www.keboola.com/ [image: Keboola LinkedIn] https://www.linkedin.com/company/keboola/?originalSubdomain=ca[image: Keboola Twitter] https://twitter.com/keboola?ref_src=twsrc%5Egoogle%7Ctwcamp%5Eserp%7Ctwgr%5Eauthor[image: Keboola YouTube] https://www.youtube.com/channel/UCUDR3znPbK9xvOHYIGBBSIA/videos[image: Keboola FB] https://www.facebook.com/KeboolaHQ/ Mobile: (+44) 7426 832065, (+420) 723 931 950 Web: www.keboola.com

DoMoreWithData

http://community.keboola.com/?utm_source=email&utm_medium=email%20signature&utm_campaign=community-pro-link

tomasfejfar commented 3 years ago

Sorry, I though UI is using the runOrchestration endpoint and allows manual runs (it might have in the past or I mixed it up with somethings else)... it makes sense to make it honor the disable status by default.