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

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

Open
derberg opened this issue Nov 9, 2022 · 16 comments
Open

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

derberg opened this issue Nov 9, 2022 · 16 comments

Comments

@derberg
Copy link
Member

derberg commented Nov 9, 2022

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

  • so Alejandra is added by default as reviewer for related PRs
  • so Alejandra has a chance to react to DevEx changes in docs that were not consulted from the "user perspective"
  • so Alejandra see if we remove something that we should not 😄

@fmvilas @mcturco @akshatnema @magicmatatjahu

wdyt?
also, what files would that be? 🤔

context: #1087

@magicmatatjahu
Copy link
Member

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
Copy link
Member

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
Copy link
Member

@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.

Copy link
Member Author

derberg commented Nov 9, 2022

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

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

  • changing search
  • modifying the way text is rendered
  • changes in navigation

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
Copy link
Member

quetzalliwrites commented Nov 9, 2022

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
Copy link

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

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 ❤️

@akshatnema
Copy link
Member

Well, @derberg Do we need this now also?

Copy link
Member

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

@derberg
Copy link
Member Author

derberg commented Mar 15, 2023

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
Copy link
Member Author

derberg commented Mar 15, 2023

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 asyncapi/generator#905

@github-actions
Copy link

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

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 ❤️

@akshatnema
Copy link
Member

@derberg Any updates on this?

@derberg
Copy link
Member Author

derberg commented Oct 2, 2023

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

@akshatnema
Copy link
Member

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

Copy link
Member Author

derberg commented Oct 3, 2023

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

@sambhavgupta0705
Copy link
Member

@derberg @akshatnema need a discussion for this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants