Adding codeowners for each command

[jonaslagoni]

Reason/Context

In Modelina we have a similar setup (where each language, input and website has sub-codeowners) which I think makes sense to have in the CLI as well.

And that is: Each command has their own set of codeowners, so that for example https://github.com/asyncapi/cli/blob/master/src/commands/generate/models.ts has me @magicmatatjahu and @kennethaasan as codeowners.

The same would apply for all the other commands as well of course.

That will lift the burden on CLI core codeowners, as most changes to those commands are unrelated to the core of the CLI and does not really need their focus.

Hope it makes sense, otherwise let me know

[ayushnau]

@jonaslagoni i would like to work on this. please project me to correct direction how can i do this. also if possible can show how it is implemented in modelina.

[jonaslagoni]

This is not something you can pickup @Ayush2020012016, this has to come from the current cli codeowners.

[ayushnau]

ok @jonaslagoni

[derberg]

So yeah, the idea makes sense, not sure though if here it can be done 100% same as in Modelina. We would probably have to first plan how to properly test commands consistency with the rules we set for CLI

[smoya]

I like the idea. My only concern is when someone makes a change that affects all or several commands (i.e. changing the Parser config or similar). Not sure how you @jonaslagoni do in Modelina. Perhaps writing some guidelines in the docs might fix it.

Let me go deep on what I mean. At least two scenarios come to my mind:

1. The PR does change anything on those commands. Then, owners will be required in the PR review automatically (by matching the file path).
2. The PR does not change anything on those commands. Then, review from those command owners won't be automatically required by Github.

In the second case, shall the author of the PR add all those command owners as reviewers? In addition, it could happen that they don't really know the whole impact of their change.

In both cases, should at least 1 owner of each command give their +1 in order to get the PR merged?

[derberg]

I'm not sure how we can do it and still make sure we are consistent across commands. I guess in Modelina you just implement interfaces on "language" level ownership. Here, things work differently, maybe it would be enough to move flags like https://github.com/asyncapi/cli/blob/master/src/commands/convert.ts#L20-L24 to separate, single location, that is maintained by core maintainers so whenever changes are introduced, they know, and the rest of implementation is under command-level maintainers? 🤔 that could be first step, later we would have to figure out errors consistency

@Souvikns wdyt? @Amzani this could be a good PR for you to onboard as core maintainer https://github.com/asyncapi/cli/pull/1220#issuecomment-1988053610 as with onboarding you we could onboard other command-level maintainers. So refactor + contrib guide that explains new setup (some can probably copy/paste from modelina)

[jonaslagoni]

Why not just say that all core CLI maintainers are also maintainers of each command?

That way they always have full control to adapt anything within the CLI while command-level owners still have full control over the commands.

[derberg]

That will lift the burden on CLI core codeowners, as most changes to those commands are unrelated to the core of the CLI and does not really need their focus.

but then ☝🏼 will not work and I will be pined in every PR

[jonaslagoni]

Hmmm, yea, that is true.

[derberg]

@jonaslagoni but this should be doable -> https://github.com/asyncapi/cli/issues/781#issuecomment-1988082703 and we anyway have @Amzani volunteering to maintain CLI and could maybe work on it

[Amzani]

Looks like a good plan, I'm rethinking and POCing a better architecture for CLI

[derberg]

@Amzani perfect, I like the new structure, so yeah, please take into account this issue, that we need some parts to be in separate files, so when configuring CODEOWNERS we can provide paths where core maintainers whenever there are cross commands/api modifications, or commands or API design is affected.

example scenarion:

• You are core maintainer

• @jonaslagoni maintains only modelina command

• if Jonas refactors code, updates some code how modelina is used, does some refactor of related tests - you are not pinged and required to approve to merge

• if Jonas refactors code and modifies that output flag is now called outputs - you are also required in review and should block merge by first having a discussion why for one command you want to rename flag that everywhere else it is called differently.

[Amzani]

Sure @derberg I'll incorporate this requirement in the new architecture of CLI, to validate my approach using the hexagonal architecture

• internal/*: Only core business logic (Parser config, Generator...)
• ports/*: They define the contract or API that the application exposes for external actors, and this will contain interfaces (e.g generator interface, CLI interface including flags)
• adapters/* Implementation of those interfaces (eg. Oclif CLI, API / REST / GRPC...)

Let's say I'm changing a foo command, or /foo endpoint to the API

Who is pinged when paths are affected?

• Internal/foo: Only foo Maintainers
• Internal/core: The change is affecting many commands, core maintainers are pinged
• ports/* : (e.g I'm renaming a flag, API change), foo Maintainers+core maintainers are pinged as the breaking change should be discussed if any, this can be automated in the future as well by introducing some breaking change rules - or using some tooling like optic-ci
• adapters/foo*: Only foo Maintainers

Notes:

• tests should be distributed and not centralized in 1 single directory
• Business logic must be extracted from current commands

cc @jonaslagoni @smoya

[derberg]

what is internal in relation to https://github.com/asyncapi/cli/pull/1200 ?

[Amzani]

@derberg I renamed internal to core to make it explicit

[smoya]

#1200

Don't wanna sound picky, but I think domain is the word you are looking for if it is the location where all domain logic (models, services, data loaders or repositories, etc) will be located into.

[github-actions[bot]]

This issue has been automatically marked as stale because it has not had recent activity :sleeping:

It will be closed in 120 days if no further activity occurs. To unstale this issue, add a comment with a detailed explanation.

There can be many reasons why some specific issue has no activity. The most probable cause is lack of time, not lack of interest. AsyncAPI Initiative is a Linux Foundation project not owned by a single for-profit company. It is a community-driven initiative ruled under open governance model.

Let us figure out together how to push this issue forward. Connect with us through one of many communication channels we established here.

Thank you for your patience :heart: