Skip to content
This repository has been archived by the owner on Nov 14, 2019. It is now read-only.

Service resolution for the workflow #125

Merged
merged 4 commits into from
Aug 21, 2019
Merged

Conversation

antho1404
Copy link
Member

@antho1404 antho1404 commented Aug 16, 2019

Instead of defining an instanceHash workflows now accept service that can be the service hash or sid.

If the instance is present then we use the instance otherwise we use the service and test that the service exists. If it exists then we get the instance of this service (throw an error if there are multiple instances), otherwise, we start a new instance of this service.

This doesn't support the creation of an instance with predefined envs yet.

@antho1404 antho1404 added the enhancement New feature or request label Aug 16, 2019
@antho1404 antho1404 added this to the next milestone Aug 16, 2019
@antho1404 antho1404 self-assigned this Aug 16, 2019
@krhubert
Copy link
Contributor

I remember that some time ago we agreed that sid shouldn't be supported on engine site as it makes some resolution pbms and other pmbs too.

The decision was to keep sid in cli local database and use it, and the service list api is more used.

I would love to see a sid local database before going on with further sid support

@antho1404
Copy link
Member Author

the resolution of sid is done by the cli here and if there is an issue on the resolution (because of multiple match) there is an error displayed that encourage to use the hash instead

e4af202#diff-595898b5510f3ae484d2b6a4a90047a8R39

Also a database of alias/sid would be great on the CLI but let's think about that in another PR/issue and it might not be necessary if we have the service api with some filters

@krhubert
Copy link
Contributor

the resolution of sid is done by the cli here and if there is an issue on the resolution (because of multiple match) there is an error displayed that encourage to use the hash instead

Resolution yes, but sids are taken from the engine still. The previous discussion was about abandoning sid in the engine, but this PR introduce more connection with engine and I think this should be the first step to finally get rid of the sid on the engine side. Except that LGTM :)

Copy link
Member

@NicolasMahe NicolasMahe left a comment

Choose a reason for hiding this comment

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

looks good but the error should be improved to give the reason how to fix it.

src/commands/workflow/compile.ts Outdated Show resolved Hide resolved
src/commands/workflow/compile.ts Outdated Show resolved Hide resolved
src/commands/workflow/compile.ts Outdated Show resolved Hide resolved
src/commands/workflow/compile.ts Show resolved Hide resolved
@NicolasMahe
Copy link
Member

the resolution of sid is done by the cli here and if there is an issue on the resolution (because of multiple match) there is an error displayed that encourage to use the hash instead

Resolution yes, but sids are taken from the engine still. The previous discussion was about abandoning sid in the engine, but this PR introduce more connection with engine and I think this should be the first step to finally get rid of the sid on the engine side. Except that LGTM :)

The Engine is not compatible with SID like before. SID is just a parameter of service. The cli (using an helper from mesg-js) is doing the resolver itself by listing services... This will be easily transformable to a local CLI's database 👍

antho1404 and others added 2 commits August 21, 2019 15:06
Co-Authored-By: Nicolas Mahé <nicolas@mesg.com>
Copy link
Member

@NicolasMahe NicolasMahe left a comment

Choose a reason for hiding this comment

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

looking good 👍

@antho1404 antho1404 merged commit 7572a1f into master Aug 21, 2019
@antho1404 antho1404 deleted the feature/workflow-resolve-sid branch August 21, 2019 08:20
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants