search

asyncapi / website

AsyncAPI specification website
https://www.asyncapi.com
Apache License 2.0
528 stars 670 forks source link

proposal to make Alejandra codeowner for docs related UI code #1089

Open derberg opened 2 years ago

derberg commented 2 years ago

To improve automation and review process imho we should make @alequetzalli a codeowner for code rated to docs view

@fmvilas @mcturco @akshatnema @magicmatatjahu

wdyt? also, what files would that be? 🤔

context: https://github.com/asyncapi/website/pull/1087

magicmatatjahu commented 2 years ago

Well it could be a problem to indicate which files, because besides the code for docs pages we should also add to other parts like roadmap and tsc page if we wanted to change the page descriptions. It would be better to add to the whole code.

magicmatatjahu commented 2 years ago

I don't know if you remember Łukasz but in a previous project we use (in project website) the json for describing texts and then this json was used during rendering - there was one json with all the texts on the website and then the parts of this json e.g. landingPage.testimonials.header were rendered on the browser side - TWs were assigned to this file and when someone added something new/change, the TWs saw these changes and had to approve the new texts. Of course, this does not solve the problem with ids but it would solve the problem with other texts on the site that do not pass review from TW perspective.

akshatnema commented 2 years ago

@derberg Yeah, we can make @alequetzalli codeowner for the docs-related UI code, but the problem arises, are we gonna categorise codeowners related to each file or category of files/folders? And there is also the case, where I get the problem that on updating/adding new components in the docs (especially in MD files), we have to take necessary review from the @alequetzalli, which doesn't mainly comprises of the changes related to Docs. So, we actually come up into the situation that on some PRs we have the mismatch of the codeowners being asked for reviews.

What I can suggest is that each codeowner whenever finds that the PR also requires the approval of another codeowner also (like @alequetzalli or @mcturco), they can add do-not-merge label in it and waits for the review approval from respective codeowner.

derberg commented 2 years ago

Just to clarify. It is not much related to "text" on the website.

It is more related to changes that affect the "docs" UI:

Also, it is not about "blocking" the merge because Alejandra will have to approve. We still have branch protection that requires only 1 approval. It is about using GitHub automation, that will add Alejandra to PR because she is one of the codeowners.

And I like what @akshatnema wrote. When @alequetzalli is added as codeowner, GH will add her to review, and she will evaluate on her own if she needs time to review this one PR as it affects documentation. So she will leave a comment that she adds /dnm because she needs to have a closer look.

How does that sound?

We can just add Alejandra to https://github.com/asyncapi/website/blob/master/CODEOWNERS#L8 but this means she will be triggered with every new PR. Not sure @alequetzalli want's it 😏

also @akshatnema we have repos where we already have codeowners per files or per folders, and it works well.

quetzalliwrites commented 2 years ago

haha yeah, I am excited to follow this convo and see what rules can be set in place to ideally add me to Docs issues for UX/DevEx/markdown/etc changes. Maybe doing it per folder? Like in the website repo we know it would be the docs folder, and then we could do that for repoes with docs?

yeah, I can also use a /dnm label for certain scenarios if that's what ya'll think should also be done 👌

github-actions[bot] commented 1 year ago

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:

akshatnema commented 1 year ago

Well, @derberg Do we need this now also?

quetzalliwrites commented 1 year ago

i mean, i still think it would be useful but i don't know the end solution 😂😄

derberg commented 1 year ago

to not complicate to much, what about if for started we make sure @alequetzalli gets notification whenever changes are done to *Docs*.js as whenever there is something docs related, any component docs related, it always have Docs prefix. That would have to be added at the end of the document. We did similar stuff here https://github.com/asyncapi/community/blob/master/CODEOWNERS#L11

That should cover majority of cases really and we will simply improve over time 🤷🏼

derberg commented 1 year ago

although technically, it has to be tested, might be it won't work nicely. yeah, we need to add it first to the list -> for reference https://github.com/asyncapi/generator/pull/905

github-actions[bot] commented 1 year ago

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:

akshatnema commented 1 year ago

@derberg Any updates on this?

derberg commented 1 year ago

@akshatnema I'm not 100% sure what is the final pattern we should put in codeowners

akshatnema commented 1 year ago

So, should we remain the same as we are right now for codeowners?

derberg commented 1 year ago

no, we should find time to test how proper pattern should look like 😃 do you think *Docs*.js is good for a start?

sambhavgupta0705 commented 4 months ago

@derberg @akshatnema need a discussion for this

github-actions[bot] commented 1 week ago

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: