search

operate-first / common

Git Hub organization configuration
GNU General Public License v3.0
6 stars 64 forks source link

Labels docs patch 02 #10

Closed quaid closed 3 years ago

quaid commented 3 years ago

This is one of the fixes I was chasing in PR #9 and it seems to originate here in the template file.

What I see in the Atom editor's Markdown render is, the header gets pulled in as a cell in an additional row in the table preceding it. Apparently GitHub's render engine doesn't do this. AIUI, the best practice is to but a blank line before and after a heading for compatibility; refer to: https://www.markdownguide.org/basic-syntax/#heading-best-practices

quaid commented 3 years ago

Atom-renders-header shows the rendering without a blank line before the header, as produced by the template file.

tumido commented 3 years ago

@quaid the labels.md are generated from labels.md.tmpl. Any manual change to labels.md will be overwritten. Can you please update the template to fix your issue?

you can easily render the template into markdown locally via:

podman run -it -v.:/mnt:Z gcr.io/k8s-prow/label_sync:latest --action=docs --config=/mnt/labels.yaml --docs-template=/mnt/labels.md.tmpl --docs-output=/mnt/labels.md
quaid commented 3 years ago

@quaid the labels.md are generated from labels.md.tmpl. Any manual change to labels.md will be overwritten. Can you please update the template to fix your issue?

Yes, that fix is in this patch in commit 221e7e1 but it also has my manual fix.

I can revert my changes to the generated file within this patch so it's just the template fix.

tumido commented 3 years ago

@quaid well.. I think we don't want any manual changes in labels.md, let's keep only what's generated from labels.md.tmpl, because any consecutive changes to the labels.yaml will lead to regenerating labels.md, hence discarding your manual changes. :shrug:

Let me try to adjust the template a bit for you, so the end result is less disruptive. Would you mind if I equalize the spaces and push that into your PR here? Or you'd rather see me open a separate one?

quaid commented 3 years ago

@quaid well.. I think we don't want any manual changes in labels.md, let's keep only what's generated from labels.md.tmpl, because any consecutive changes to the labels.yaml will lead to regenerating labels.md, hence discarding your manual changes. shrug

Thanks @tumido, I'm clear on that. I figured from the beginning the file was generated (and didn't notice the .tmpl file right there, oops!), so the manual changes were always likely to be thrown away. My comment above was me asking if the best path is to delete my patch to the generated file from the commit?

... and I've tried a few ways to revert that commit within this patch, but no luck. Suggestions?

Let me try to adjust the template a bit for you, so the end result is less disruptive. Would you mind if I equalize the spaces and push that into your PR here? Or you'd rather see me open a separate one?

For sure! I definitely want understand better, and am comfortable with shared editing of things. (Plus we have a commit history, we can always roll back, etc.) Since I didn't (then) know how to test ... OK, that podman test didn't work for me, so I'm still flying blind if my template changes are sufficient or not.

tumido commented 3 years ago

Hey @quaid sorry for a late response, I'm back from PTO now.

... and I've tried a few ways to revert that commit within this patch, but no luck. Suggestions?

Hm... No idea why it doesn't work for you. Maybe the merge from master instead of rebase changed things around for you? I think you can always reset and cherry-pick commits and then push. I'll try to do something about it for you.

For sure! I definitely want understand better, and am comfortable with shared editing of things. (Plus we have a commit history, we can always roll back, etc.) Since I didn't (then) know how to test ...

Looking at it now :slightly_smiling_face:

OK, that podman test didn't work for me, so I'm still flying blind

:frowning_face: That's a bummer. Let me try to break the podman command into pieces you then, hopefully it helps you spot the error or setup difference. The command should work the same no matter if executed via podman or docker though.

podman run 
  -i # Keep STDIN open even if not attached
  -t # Allocate a pseudo-TTY for container
  -v.:/mnt:Z  # Mount "." current folder to "/mnt" within the container runtime
  gcr.io/k8s-prow/label_sync:latest # Executed image
  --action=docs # Param for the image command selecting the "docs" action which means "generate labels"
  --config=/mnt/labels.yaml # Specify the labels config file
  --docs-template=/mnt/labels.md.tmpl # Specify the template file
  --docs-output=/mnt/labels.md # And specify the output

The only place where the command above can go sideways is the -v.:/mnt:Z I think. I suggest you check out this article on the volumes. It's the only one I've been able to understand on this topic https://www.tutorialworks.com/podman-rootless-volumes/

tumido commented 3 years ago

So.. I've played with the git rebase a bit and rebased this PR on top of main, dropped the labels.md commit and slapped one more commit on top of it with the template changes. Now the labels.md are generated from the template. Let me know if the structure and changes matches your expectations.

Tip: Git commit messages are treated weirdly by git. If you want to have a long commit description you should use this format:

Commit title

Long commit
description
can be multiple lines

The empty line between title and description is important, otherwise it's all treated as a title (you didn't have the empty line in there). This is a good article on that: https://chris.beams.io/posts/git-commit/

tumido commented 3 years ago

@quaid once https://github.com/operate-first/toolbox/pull/41 is merged and released you should be able to do:

$ toolbox enter opf-toolbox-v0.4.1

/bin/sh: line 1: /bin/zsh: No such file or directory
Error: command /bin/zsh not found in container opf-toolbox-v0.4.1
Using /bin/bash instead.
⬢[tcoufal@toolbox common]$ labels_sync --action=docs --config=labels.yaml  --docs-template=labels.md.tmpl --docs-output=labels.md

the labels_sync binary will be packaged within the toolbox.

quaid commented 3 years ago

So.. I've played with the git rebase a bit and rebased this PR on top of main, dropped the labels.md commit and slapped one more commit on top of it with the template changes. Now the labels.md are generated from the template. Let me know if the structure and changes matches your expectations.

Tip: Git commit messages are treated weirdly by git. If you want to have a long commit description you should use this format:

Commit title

Long commit
description
can be multiple lines

The empty line between title and description is important, otherwise it's all treated as a title (you didn't have the empty line in there). This is a good article on that: https://chris.beams.io/posts/git-commit/

Ah, that's an important tip. I think I've got most of the rest of the tips (except for forgetting to reformat at 72 char lines) but I did not know about the lack of space after the subject causing it all to run togethre.

tumido commented 3 years ago

Sorry, this issue got autoclosed. Reopening, because I think it's a valid PR that should be merged.

tumido commented 3 years ago

@quaid can you please review this PR? I think now it achieves what you wanted initially. I'll look into the CI generation of labels in #11 right now its manual...

sesheta commented 3 years ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: tumido

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files: - ~~[OWNERS](https://github.com/operate-first/common/blob/main/OWNERS)~~ [tumido] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment