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

Remove Madoko-specific documentation from docs/README.md #537

Merged
merged 1 commit into from
Feb 14, 2025

Conversation

jafingerhut
Copy link
Contributor

No description provided.

@jafingerhut
Copy link
Contributor Author

With this PR, there are only 2 occurrences of the string "madoko" in the entire repo, and they are both in comments in the AsciiDoc source of the spec.

There are definitely no occurrences of the string "madoko" (case-insensitive search) in the .github directory.

Thus my best guess for why this repo's CI has a madoko-lint job is that someone somehow configured that in the Github web page UI somewhere. I have no idea where, though. @anontinbas Sorry to ping you, but do you have any guess on that?

@antoninbas
Copy link
Member

@jafingerhut I don't have admin permissions for this repo anymore. But if you go into the branch protection settings, you can remove madoko-lint as a required workflow for PRs. You can also add new required workflows which have been added recently, such as asciidoc-lint.

@jafingerhut
Copy link
Contributor Author

jafingerhut commented Jan 22, 2025

Thanks! Those keywords were enough clues for me to find madoko-lint in Github Settings, and delete it.

I have not yet tried adding something for asciidoc-lint. Perhaps that can be done by modifying files in .github/workflows? I don't know.

Update: I do see asciidoc-lint in the list of checks run on this and another PR already, so no further changes should be required to enable that.

@antoninbas
Copy link
Member

I have not yet tried adding something for asciidoc-lint. Perhaps that can be done by modifying files in .github/workflows? I don't know.

That would be in the same place, in the branch protection settings. Usually it will auto-display workflows that have run recently (or you just type in the beginning of the name and it will show up) and you just have to select it.

@jafingerhut
Copy link
Contributor Author

I have not yet tried adding something for asciidoc-lint. Perhaps that can be done by modifying files in .github/workflows? I don't know.

That would be in the same place, in the branch protection settings. Usually it will auto-display workflows that have run recently (or you just type in the beginning of the name and it will show up) and you just have to select it.

Thanks for the suggestion. After I wrote the above that you replied to, I noticed that the CI checks already run asciidoc-lint, I believe because of the contents of one of the .github/workflows/*.yml files, so we should be good to go now.

@antoninbas
Copy link
Member

Thanks for the suggestion. After I wrote the above that you replied to, I noticed that the CI checks already run asciidoc-lint, I believe because of the contents of one of the .github/workflows/*.yml files, so we should be good to go now.

These are 2 different things: the workflow will always run as long as there is the proper yml file with the proper triggers; but branch protection settings let your enforce that a workflow must run successfully and will prevent you from merging the PR otherwise. This avoids errors where someone merges the PR before the workflow has a chance to run or fails to see that the workflow failed.

You can see that we currently have 3 required workflows and 2 non-required workflows:
image

asciidoc-lint and DCO should probably be added to the required workflows.

@jafingerhut
Copy link
Contributor Author

@Dscano No urgency on reviewing this, but if you get a little bit of time that would be appreciated.

I think it is OK if we don't replace all of the details about how to do things in Madoko with the corresponding instructions on how to do them in AsciiDoc, personally. As long as there is an example similar to something you want to do already in the document, that is how I learn how to do things, and I suspect most other people would do the same when adding new things to the spec.

Copy link
Collaborator

@chrispsommers chrispsommers left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@chrispsommers chrispsommers left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@smolkaj smolkaj left a comment

Choose a reason for hiding this comment

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

LGTM too

@chrispsommers chrispsommers merged commit c91c52d into p4lang:main Feb 14, 2025
5 checks passed
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

Successfully merging this pull request may close these issues.

4 participants