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

build: add patch code of conduct workflow #972

Merged
merged 2 commits into from
Apr 3, 2023

Conversation

anonrig
Copy link
Member

@anonrig anonrig commented Nov 22, 2022

Please be aware that this is highly experimental, and might/will have some bugs. Couple of notes:

  • The code, documentation, and tests live inside https://github.com/anonrig/patch-my-code-of-conduct (for now)
  • When this pull request is merged, it will automatically open a pull request to update CODE_OF_CONDUCT.md file with the patch applied.
  • Before merging this pull-request, we need to get aligned with the appropriate patch file.

If we are aligned & happy on this path, I'll be happy to move the project under openjs-foundation organization.

Copy link
Member

@ovflowd ovflowd left a comment

Choose a reason for hiding this comment

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

:shipit:

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

Isn’t the point of the base that it’s a 1:1 copy of what’s in the upstream? meaning the base shouldn’t even need to be committed, since it can be fetched by sha

@tobie
Copy link
Contributor

tobie commented Nov 27, 2022

Isn’t the point of the base that it’s a 1:1 copy of what’s in the upstream? meaning the base shouldn’t even need to be committed, since it can be fetched by sha

That's an interesting point. On one hand, I'd like us to be super clear as to what we update to, on the other hand, including the non-patched CoC in our codebase sounds like a recipe for confusion. I think I'd be in favor of just fetching it.

@anonrig
Copy link
Member Author

anonrig commented Nov 29, 2022

Isn’t the point of the base that it’s a 1:1 copy of what’s in the upstream? meaning the base shouldn’t even need to be committed, since it can be fetched by sha

@ljharb What particular SHA do you have in mind? And I didn't quite understand the reasoning behind this. If we are fetching it with a constant SHA, why complicate the infra and introduce magic to future reviewers & maintainers of this package?

That's an interesting point. On one hand, I'd like us to be super clear as to what we update to, on the other hand, including the non-patched CoC in our codebase sounds like a recipe for confusion. I think I'd be in favor of just fetching it.

@tobie I'm in favor of progressing as it is, but if this is the reached consensus, I'll be happy to update the script to fetch from a specific commit sha.

@ljharb
Copy link
Member

ljharb commented Nov 29, 2022

@anonrig whichever SHA of the upstream CoC we're basing the patch onto. The goal was so the final document would be diffed along with the patch, so we could differentiate between an upstream change and an in-repo change. Inlining the upstream text is redundant.

@tobie
Copy link
Contributor

tobie commented Dec 1, 2022

Inlining the upstream text is redundant.

I'm more concerned about the fact that it introduces potential for confusion, but the outcome is the same. We should treat this as an external resource and not vendor-it in.

@anonrig
Copy link
Member Author

anonrig commented Dec 12, 2022

I moved the project to pkgjs organization: https://github.com/pkgjs/patch-my-code-of-conduct

@anonrig
Copy link
Member Author

anonrig commented Jan 4, 2023

@joesepi and I updated the code of conduct script and released 1.1.0 version. The only pre-requisite right now, which is also a blocker, is: we need to land this pull request in order to create an immutable reference to it, which will later be used as a base_url for the github workflow script.

doc/CODE_OF_CONDUCT_BASE.md Outdated Show resolved Hide resolved
@joesepi joesepi mentioned this pull request Jan 5, 2023
@anonrig anonrig force-pushed the anonrig/code-of-conduct branch 2 times, most recently from c8be85b to a7433f5 Compare January 24, 2023 17:57
@anonrig anonrig force-pushed the anonrig/code-of-conduct branch 3 times, most recently from 38b345f to 0f2ea6c Compare January 31, 2023 18:26
Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

assuming there's no CoC diffs from this, LGTM

.github/workflows/patch-code-of-conduct.yml Outdated Show resolved Hide resolved
@tobie
Copy link
Contributor

tobie commented Feb 7, 2023

I’m a little bit confused why the diff file is so big. We’re technically just patching a single word.

Am I missing something?

@anonrig
Copy link
Member Author

anonrig commented Feb 7, 2023

I’m a little bit confused why the diff file is so big. We’re technically just patching a single word.

@tobie Compared to the original document (referring: https://www.contributor-covenant.org/version/2/1/code_of_conduct/code_of_conduct.md), the current code of conduct does a little bit more than patching a single word. For example, the original document has a character limit of 120 (estimated) characters and later continues the sentence in a new line, but we don't. Due to this major change, it feels like we are patching more than that.

@anonrig
Copy link
Member Author

anonrig commented Feb 14, 2023

Do we have a sense of what still needs to be done, and what "maintenance cost" we'll need moving forward? E.g. what it patching fails, can we do manual merges, for example?

@tobie If patching fails the patch-my-coc script will not create a pull request, and it will be logged inside the github actions. If this was related due to an error in the script it needs to be changed there, but it might very well be caused by the latest included change, and in that particular part, patch script comes with macOS and linux does not give meaningful errors and it might require us to dig deep into finding the issue.

@anonrig
Copy link
Member Author

anonrig commented Feb 14, 2023

@eemeli, I’m fine with not-merging & closing this pull request. A lot of work has gone into this patching effort (not just my code here), but if we decide it's not the best way forward, that’s fine.

But I do think it would be good for us to clearly lay out the options to move forward because this has been an issue for quite some time.

To help us compare which approach to take, here is how this patching script will work:
If we want to update to a newer version of the Contributor Covenant CoC, we modify the template URL in the GitHub workflow which kicks off the process of fetching upstream, applying prefix and suffix, and writing a new ./CODE_OF_CONDUCT.md file within a pull request. This should be simple and without issue, but if there is an issue, the code isn’t too complicated to debug.

I believe the alternative is manually copying and pasting bits and pieces together into a pull request, which is fine, but also could be prone to issues.

@Relequestual
Copy link
Contributor

If I've read this right, this PR needs to be rebased now that #1007 is merged?

@anonrig anonrig force-pushed the anonrig/code-of-conduct branch 2 times, most recently from b1f3f5a to 1db63b6 Compare March 7, 2023 17:49
@anonrig
Copy link
Member Author

anonrig commented Mar 7, 2023

I updated the patch file with the necessary changes and applied the requested ones. Appreciate any reviews. Thank you.

Copy link
Member

@eemeli eemeli left a comment

Choose a reason for hiding this comment

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

Better, but still needs some work. Most importantly, to reiterate myself from before:

At no point should we be removing the ./CODE_OF_CONDUCT.md file from our repo.

conduct/artifacts/CODE_OF_CONDUCT_PATCH.patch Outdated Show resolved Hide resolved
conduct/artifacts/CODE_OF_CONDUCT_PATCH.patch Outdated Show resolved Hide resolved
conduct/artifacts/CODE_OF_CONDUCT_PATCH.patch Outdated Show resolved Hide resolved
@anonrig
Copy link
Member Author

anonrig commented Mar 10, 2023

@eemeli, I've updated the pull request with the following changes. Thank you for your patience and your reviews. I greatly appreciate it!

  • Updated references in the COC.md for v2.1 (which the script uses)
  • Removed all invalid changes on the patch file.
  • Removed the commit for removing the existing CODE_OF_CONDUCT.md

@tobie
Copy link
Contributor

tobie commented Mar 10, 2023

That looks a lot cleaner. Thanks!

I'm curious what the process for changing the patch would look like.

@anonrig
Copy link
Member Author

anonrig commented Mar 10, 2023

I'm curious what the process for changing the patch would look like.

@tobie Maybe someone can have a better solution for this, but locally, I have two files. One only contains PREAMBLE + CoC (original one), and the other: all changes applied file (which will be CODE_OF_CONDUCT.md in the future). I run diff -Naur ./current.md ./expected.md > PATCH_FILE.patch to generate the patch file.

@Relequestual Relequestual linked an issue Mar 10, 2023 that may be closed by this pull request
@anonrig anonrig force-pushed the anonrig/code-of-conduct branch 2 times, most recently from 8798790 to 07251a6 Compare March 13, 2023 20:55
Copy link
Member

@eemeli eemeli left a comment

Choose a reason for hiding this comment

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

🎉

@anonrig anonrig merged commit c4daeb6 into openjs-foundation:main Apr 3, 2023
@anonrig anonrig deleted the anonrig/code-of-conduct branch April 3, 2023 13:20
@anonrig
Copy link
Member Author

anonrig commented Apr 3, 2023

I'll open a follow up draft PR for documenting the CoC patching flow.

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.

Add patching script for CoC modifications
8 participants