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

Suggestion: get rid of context or making it optional #34

Closed
fmvilas opened this issue Jul 23, 2021 · 15 comments
Closed

Suggestion: get rid of context or making it optional #34

fmvilas opened this issue Jul 23, 2021 · 15 comments

Comments

@fmvilas
Copy link
Member

fmvilas commented Jul 23, 2021

Disclaimer: Don't take the tone of this issue too seriously, it's just a representation of a user's frustration, not necessarily mine 😄 Read this as a story.


Alright! AsyncAPI has a CLI, this is cool. I'm gonna install it and validate my file.

npm install -g @asyncapi/cli

Lovely. Let's now validate my AsyncAPI file:

$ asyncapi validate
No contexts saved yet, run asyncapi --help to know more.

Oh! My fault, I have to provide the file name:

$ asyncapi validate asyncapi.yml
No contexts saved yet, run asyncapi --help to know more.

❗ 😕

context? what the f*ck is a context? what do they mean by context? ok, let me try with asyncapi --help:

(redacted)
       context
                current  show the current set context
                list     show the list of all stored context
                remove  <context-name> remove a context from the store
                use <context-name>  set any context as current
                add <context-name> <filepath> add/update new context

  Examples
        $ asyncapi context add dummy ./asyncapi.yml
        $ asyncapi validate --context=dummy
        $ asyncapi validate --file=./asyncapi.yml

🤦 Ok, now I know I can create contexts, list contexts, remove contexts, and obviously add them. But I still don't know what is a "context".

HOW DO I VALIDATE MY FILE FOR GOD SAKE!? :rage1:

Oh, wait, from the examples I can guess I need to add my file to a "context" and then run asyncapi validate --context=my-context. Let's do that.

Cool! How do I name my context? Let me think about it well so I don't create a stupid context name for later. Or maybe I can just use "test" for now. I wonder where this information is stored 🤔

Anyway, let's just use "test"

asyncapi context add test ./asyncapi.yml
asyncapi validate

Cool, it works now. Oh wait, there was a shorter way! 🤦 I could just have done:

asyncapi validate --file=./asyncapi.yml

Do you see where I'm going? I agree contexts can be useful for experienced and regular users but for newcomers, having to deal with the burden of contexts for simple things like validating a file is such a huge barrier that shouldn't exist.

My proposal is that we remove contexts completely. I think they may make sense in the future —who knows?— but for now, this is only making things more difficult to reason about. Instead, we can rely on a convention over and then configuration style. For instance:

asyncapi validate

Would look for files with the name asyncapi.yml, asyncapi.yaml, or asyncapi.json in the current directory. If it's found, it validates the file. If it's not, it fails and shows a message saying it couldn't find an asyncapi file and suggesting how to continue.

Following what we discussed on #1, I think asyncapi verb noun should be respected. The noun may be optional when we can easily default to something. Like this case or the case of asyncapi new which could be the equivalent of asyncapi new file. I'm always in favor of options/flags over arguments but if an option is mandatory, it's not an option.

But Fran, my file is called something else, like spec.yml and I still want to benefit from short commands

Cool, now we introduce contexts. The user is going to enable the "contexts" feature knowing what they're doing and on purpose.

$ asyncapi enable contexts
Awesome! From now on, I'll use the `default` context.

Please add your AsyncAPI file to the context:

asyncapi update context your-asyncapi-file.yml
$ asyncapi update context spec.yml
✅  Your AsyncAPI file (spec.yml) has been added to the `default` context.

Note I keep using the pattern asyncapi verb noun and I'm always trying to go ahead of the user. For instance, I decided there's going to be a default context (eliminating the need to think about a name). Users should be able to customize the name if they prefer, doing asyncapi enable contexts --context=my-context-name. With asyncapi update context your-asyncapi-file.yml, we could even go further and make the file name optional, and if you run asyncapi update context the CLI will prompt you a file name or fail if stdin is not interactive.

Summary

In short, I think we should be making the UX seamless for newcomers and let experienced and avid users to get the most out of the CLI by letting them configure the behavior. Let's assume defaults and conventions whenever possible. Let's make them configurable via options/flags or interactive terminal when possible.

@github-actions
Copy link
Contributor

Welcome to AsyncAPI. Thanks a lot for reporting your first issue. Please check out our contributors guide and the instructions about a basic recommended setup useful for opening a pull request.

Keep in mind there are also other channels you can use to interact with AsyncAPI community. For more details check out this issue.

@MikeRalphson
Copy link

MikeRalphson commented Jul 23, 2021

Or at least introduce the concept of a default context, which a simple / single input document will use. I think I understand the use of a context for related resources, but I'm unsure why I would want more than one context in a single CLI 'session'.

@Souvikns
Copy link
Member

I really like the idea about enabling contexts, at it removes confusion for the newcomers but also keeps it for the experince users to leverage it if they want it.

@fmvilas
Copy link
Member Author

fmvilas commented Jul 25, 2021

Or at least introduce the concept of a default context

Yes, sorry, I had that in the issue title but somehow lost it. Changing it.

@fmvilas fmvilas changed the title Suggestion: get rid of context Suggestion: get rid of context or making it optional Jul 25, 2021
@derberg
Copy link
Member

derberg commented Jul 26, 2021

Ouch, lots of frustrations here 😅 tell this imaginary user that I recommend 🍵 with a bit of lemon balm 😆

Keep in mind this CLI is in a super early stage of development at 0.3 release and many basic improvements are missing (like good UX messages to the user). Of course long term instead of No contexts saved yet, run asyncapi --help to know more. we should be much more descriptive, explaining what are the possible options if defaults were not met.

Context purpose is to be optional, not default. Except of WebSocket use case, developers work in many cases with more than just one AsyncAPI file. Contexts are to support such work, this is why you want to be able to easily switch between files with their names instead of remembering paths.

Contexts are not optional as they are not completed feature. The --file flag was added in 0.3.

Would look for files with the name asyncapi.yml, asyncapi.yaml, or asyncapi.json in the current directory. If it's found, it validates the file. If it's not, it fails and shows a message saying it couldn't find an asyncapi file and suggesting how to continue.

This is exactly what we discussed with @Souvikns on his initial PR for contexts but did not want to over-complicate the PR, same was about --file flag. This functionality will follow. Plan is -> if --file is not passed, and if --context is not passed, and if there is no default context already set that could be used, we look for asyncapi.yml, asyncapi.yaml, or asyncapi.json in the current directory, otherwise help message with good explanation of possible options.

So, too summarize, we do have UX in mind, but please do not expect all good things at 0.3 version 😉

PS. about verb noun vs noun verb, it is not related to context itself, so I suggest you report a separate issue if you want to discuss further.

@derberg
Copy link
Member

derberg commented Jul 26, 2021

btw, I actually checked and asyncapi.yml, asyncapi.yaml, or asyncapi.json in the current directory is already supported in 0.3 version. @Souvikns you are fast 🥳 💪🏼

@magicmatatjahu
Copy link
Member

Referring to the Łukasz's comment, we can treat the AsyncAPI docs as the first-citizens of the our CLI, so passing them not as flags, but as standalone arguments. Idea is described here.

@fmvilas
Copy link
Member Author

fmvilas commented Jul 26, 2021

Well, first of all I'd like to apologize if someone has felt offended by the tone of the message. Please don't take this personally as an attack to what's been developed so far 🙏 I thought it'd be funny to add some example of a raging user, that's why the all caps and the :rage1: and 🤦 emojis. I'm very aware this is 0.3.0 (very early stage) and not judging your work, especially @Souvikns and @jotamusik who have been working a lot on this project. I'm sorry if this has offended any of you 🙏

we should be much more descriptive, explaining what are the possible options if defaults were not met

Agree. Just keep in mind that when you have to explain too much, it means something is failing in the UX.

Except of WebSocket use case, developers work in many cases with more than just one AsyncAPI file. Contexts are to support such work, this is why you want to be able to easily switch between files with their names instead of remembering paths.

You'd have to explain this a bit more to me. Why is it different when using Websockets? 🤔

@derberg
Copy link
Member

derberg commented Jul 26, 2021

With WebSocket you deal with one backend, one AsyncAPI file. With other architectures, broker-centric for example, you deal with more than just one microservice, so multiple files. Example https://github.com/amadeus4dev/amadeus-async-flight-status where you have 3 services and therefore 3 files. You do not work with these files every day, most likely from time to time, so you add them to context so it is easier to find and go back.

For such case you work like this

$ asyncapi context list
monitor : /Users/me/sources/amadeus-async-flight-status/monitor/asyncapi.yaml
notifier : /Users/me/sources/amadeus-async-flight-status/notifier/asyncapi.yaml
subscriber : /Users/me/sources/amadeus-async-flight-status/subscriber/asyncapi.yaml
$ asyncapi context use notifier
$ asyncapi validate
$ asyncapi list channels
$ asyncapi add channel
$ asyncapi ...

Otherwise you work like

$ asyncapi validate /Users/me/sources/amadeus-async-flight-status/notifier/asyncapi.yaml
$ asyncapi list channels /Users/me/sources/amadeus-async-flight-status/notifier/asyncapi.yaml
$ asyncapi add channel /Users/me/sources/amadeus-async-flight-status/notifier/asyncapi.yaml

or like

$ asyncapi validate --file /Users/me/sources/amadeus-async-flight-status/notifier/asyncapi.yaml
$ asyncapi list channels --file /Users/me/sources/amadeus-async-flight-status/notifier/asyncapi.yaml
$ asyncapi add channel --file /Users/me/sources/amadeus-async-flight-status/notifier/asyncapi.yaml

Does it make sense now?
And anyway, contexts are optional, you do not need them, you have --file or asyncapi.yml, asyncapi.yaml, or asyncapi.json in the current directory

@fmvilas
Copy link
Member Author

fmvilas commented Jul 26, 2021

Oh, I see. Thanks for explaining.

or asyncapi.yml, asyncapi.yaml, or asyncapi.json in the current directory

That's not how it's working. Maybe how it's intended to work?

Captura de pantalla 2021-07-26 a las 13 33 45

You do not work with these files every day, most likely from time to time, so you add them to context so it is easier to find and go back.

Just to make a point regarding this. If it's something you only use "from time to time" it seems way too much to create a feature like contexts. It overcomplicates the CLI unnecessarily IMHO (implementation and usage). When I'm working with different microservices at the same time, I usually have different terminals open because I anyway need them to start, stop, or reload the microservice. But that's probably just how I work.

It seems the idea is taken from the kubectl CLI. I think it makes sense there because you open a terminal and become an operator for different clusters that are usually defined in the same k8s config file. Therefore, you don't have this option of changing to another directory because the other contexts are defined in the same k8s file. I know, it's not always like this but it's a common option and you have to take it into consideration (and they did it, good for them). Of course, you can always open another terminal and operate each cluster on a terminal but that's not convenient because you'd have to open a terminal exclusively for operating a given cluster. Hence, contexts is a clever feature. However, with AsyncAPI, an asyncapi.yaml file defines only one service and you will anyway need to open another terminal to start/stop the service (if you're working with it, of course).

In short, the more I think about contexts, the more I think the feature is not justified here. Will it make sense in the future? Probably. Or probably not. I'm just seeing this as a very early optimization. Precisely because it's 0.3.0, I think this very edge-case shouldn't be considered yet.

@fmvilas
Copy link
Member Author

fmvilas commented Jul 26, 2021

Nonetheless, I see a case where contexts could be useful tho. For those tools that need multiple asyncapi files as the input, it would be super useful. For instance, if we wanted to leverage https://github.com/asyncapi/app-relations-discovery from the CLI, having to pass a bunch of asyncapi files each time is not really convenient. In such case, having something like a context would be super cool but it should allow us to add more than one asyncapi file to the context.

asyncapi create context --name=graph
asyncapi add file ./service/dummy1/asyncapi.yaml
asyncapi add file ./service/dummy2/asyncapi.yaml
asyncapi add file ./service/dummy3/asyncapi.yaml
asyncapi create graph

@derberg
Copy link
Member

derberg commented Jul 26, 2021

Contexts feature is there already, optional, I don't see a point of removing it really.

but it should allow us to add more than one asyncapi file to the context

As you can see in my example, you have multiple files in the context because you can add multiple contexts, just one can be the default of course:

$ asyncapi context list
monitor : /Users/me/sources/amadeus-async-flight-status/monitor/asyncapi.yaml
notifier : /Users/me/sources/amadeus-async-flight-status/notifier/asyncapi.yaml
subscriber : /Users/me/sources/amadeus-async-flight-status/subscriber/asyncapi.yaml

Using some kind of name as you showed in your example, or rather a label could be good. Something to consider when we integrate relations discovery, or diff.

That's not how it's working. Maybe how it's intended to work?

It works on my side 😄 but I have some files in my context. You found a bug #35

Precisely because it's 0.3.0, I think this very edge-case shouldn't be considered yet.

it took some time discussing the initial CLI setup (#1) and there were no objections for context (that I've put there from the very beginning, clearly stating kubectl inspiration). As I showed, in broker-centric arch it is useful and therefore not edge-case really.

When I'm working with different microservices at the same time, I usually have different terminals open because I anyway need them to start, stop, or reload the microservice. But that's probably just how I work.

The current most common practice that I'm aware of is that people prefer mono repo (like in the case of amadeus showcase) and definitely not open and start each service one by one in a separate terminal but use docker-compose to make your life easier, thus single terminal window. I don't believe we should consider such a flow of working as an edge-case

@fmvilas
Copy link
Member Author

fmvilas commented Jul 26, 2021

As you can see in my example, you have multiple files in the context because you can add multiple contexts, just one can be the default of course:

🤔 Now I'm more confused then. These are different contexts, right? What I mean is having a single context with multiple asyncapi files. I don't think we're meaning the same.

it took some time discussing the initial CLI setup (#1) and there were no objections for context

To be honest, I always understood contexts as a way to have multiple files as the input for a command. That's why I thought it would be cool.

but use docker-compose to make your life easier, thus single terminal window

This is a good argument for contexts, I agree 👍

@fmvilas
Copy link
Member Author

fmvilas commented Jul 26, 2021

PS. about verb noun vs noun verb, it is not related to context itself, so I suggest you report a separate issue if you want to discuss further.

I opened an issue: #37

@fmvilas
Copy link
Member Author

fmvilas commented Jul 26, 2021

Alright, so I think this issue doesn't make sense anymore as it seems it was already the intention to make it optional and a good and frequent use-case (when using docker-compose) has been argued. Other issues have been created to fix the exposed problems. I'm closing it but feel free to reopen if you think there's still a case.

@fmvilas fmvilas closed this as completed Jul 26, 2021
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

No branches or pull requests

5 participants