Contributing Guide

[SteVwonder]

Copy over the Contributing Guide from https://github.com/ExaWorks/job-api-spec, add in more bits from C4, and copy the summary from matplotlib's dev workflow.

[dongahn]

Looks like a great start. Just two things. I think it would be beneficial to make the "requirement" vs. "preference" language clear. I'd suggest to add

Language

The key words "MUST", "MUST NOT", "REQUIRED", "SHALL", "SHALL NOT", "SHOULD", "SHOULD NOT", "RECOMMENDED", "MAY", and "OPTIONAL" in this document are to be interpreted as described in RFC 2119.

I believe you already use the capitalized words for the addition. But please adjust the original copy from job-api-spec with these where applicable.

[kylechard]

I would vote against the RFC language as I think it is less welcoming to a diverse range of developers.

The summary is a nice description of the process. I'm not sure we need as much of the additional text as it doesn't really add a lot of clarification and uses different terminology to describe similar concepts.

[dongahn]

@kylechard: Good point. I can go either way though as a contributor myself I'm more on the "more explicit language" side especially when that language was used for a wide range of projects and standardization (e.g., for the creation of Internet https://tools.ietf.org/html/rfc2119). Others?

[SteVwonder]

The summary is a nice description of the process. I'm not sure we need as much of the additional text as it doesn't really add a lot of clarification and uses different terminology to describe similar concepts.

Fair point about the different terminology and repetition. I think the big things to include in the summary if we drop the RFC-style sections are:

• Minor details about commit message contents (keep title short, put thorough description below it, reference issues in commit body)
• Using WIP PRs if you want early feedback on your changes

If we drop the RFC-style sections from contributing.md, I think we should still put the "PR Merging Process" section somewhere in the repo (doesn't have to be in Contributing.md). I think that section is important for maintainers - both to have it written down and for the maintainers to agree on that process.

Thoughts?

[andre-merzky]

If we drop the RFC-style sections from contributing.md, I think we should still put the "PR Merging Process" section somewhere in the repo (doesn't have to be in Contributing.md). I think that section is important for maintainers - both to have it written down and for the maintainers to agree on that process.

+1 on that part specifically.

[jameshcorbett]

I agree with Kyle about removing the RFC language. I was a bit put off when I first tried to contribute to Flux and was referred to the contribution guidelines---the length, the wording, and the capitalization were a little intimidating. I certainly prefer the matplotlib/numpy/cpython style of contribution guide.

I like the idea of adding some bullets to the summary and dropping/moving the other sections.

[kylechard]

Minor details about commit message contents (keep title short, put thorough description below it, reference issues in commit body) Using WIP PRs if you want early feedback on your changes

Agreed on both.

If we drop the RFC-style sections from contributing.md, I think we should still put the "PR Merging Process" section somewhere in the repo (doesn't have to be in Contributing.md). I think that section is important for maintainers - both to have it written down and for the maintainers to agree on that process.

Good point. Yea, we should state that here too.

[SteVwonder]

Ok. Take 2. Just pushed another version based on the great feedback. Let me know what you think.

(Happy to squash everything down before merging if that is desired).

[dongahn]

@SteVwonder: in this case keeping the intermediate commits make sense since they reflect the line of thinking from the contributors. Once you get one more approval, this can go in. Any one want to take a look?

[jameshcorbett]

Looks good to me too.

[dongahn]

Great. So just one more change before this can be merged. I realized I probably want to see if Addi from the ExaAM team has any feedback for this. She expressed her interest in engaging our team earlier.

[hategan]

I was looking for the contributing branch only to realize that this is a fork.

Is there a way to nicely deal with this issue without having to do a fresh clone?

[dongahn]

@hategan:

git remote add herbein his_sdk_repo_url Then you can git fetch herbein. You should be able to get to his branch through herbein remote.

[dongahn]

FYI -- I will wait a couple more days in case there are some external feedback before merging this in.

[SteVwonder]

In #35, I started to add a merge bot with acceptance rules based on the current proposed contributing guide. The ruleset is pretty similar to Flux's merge bot's config, but since Flux follows C4, that merge bot only merges PRs that can be cleanly rebased onto the master branch. A contributor having a merge commit (which would not cleanly rebase onto the master branch) in their PR is technically allowed by the current proposed contributing guide. In that scenario, I see three options:

1. We fallback to merging (while still encouraging regular contributors to use the rebase workflow so that merging is rare - this is also what is proposed in #35)
2. We modify the contributing guide to require rebases/disallow merges and require the contributor to rebase their PR, dropping any merge commits (potentially daunting task for anyone unfamiliar with git and rebasing)
3. We have the maintainers take on the responsibility of resolving conflicts during a rebase (seems like the worst option)

Thoughts?

[dongahn]

If we allow a merge commit on the main branch, what happens to the status of the head of the main branch once the automatic merge commit is done? Is that guaranteed to work? If so, this works for me. But if we don't have that guarantee, I'd worry it can significantly lower programmer productivity. When I come to a repo, I always assume the head of the main works.

[SteVwonder]

When I come to a repo, I always assume the head of the main works.

Totally agree. Having a CI is pointless if your main branch doesn't have to pass it.

If we allow a merge commit on the main branch, what happens to the status of the head of the main branch once the automatic merge commit is done?

I assumed Mergify does not merge PRs that conflict with the base branch, but it turns out there is a boolean to control that. I can add that conflict condition to ensure that PRs with conflicts will not automatically merged (i.e., contributor or maintainer has to resolve the conflict).

Even without that condition, I know that on the Flux repos, Mergify will first perform a rebase, force-push the changes (if no conflicts), and wait until the CI goes green again before merging. I assume the same is true for a conflict-free merge (it does the merge and then confirms that the CI goes green first before the final merge via GH).

[dongahn]

I assumed Mergify does not merge PRs that conflict with the base branch, but it turns out there is a boolean to control that. I can add that conflict condition to ensure that PRs with conflicts will not automatically merged (i.e., contributor or maintainer has to resolve the conflict).

Cool

Even without that condition, I know that on the Flux repos, Mergify will first perform a rebase, force-push the changes (if no conflicts), and wait until the CI goes green again before merging. I assume the same is true for a conflict-free merge (it does the merge and then confirms that the CI goes green first before the final merge via GH).

Yes this definitely needs to be the case. Once that gets in, we probably want to test when our repo doesn't have whole lot just yet...

[dongahn]

@SteVwonder: W/ mergefy PR in, this PR can be merged with further modification? Other revisions can come in later, but from my perspective, this looks great.

Before merging this, we should fix the CI failure. Is that just rebase + force push?

[SteVwonder]

@mergifyio rebase

[mergify[bot]]

Command rebase: success

Branch has been successfully rebased

[SteVwonder]

@SteVwonder: W/ mergefy PR in, this PR can be merged with further modification? Other revisions can come in later, but from my perspective, this looks great.

Yeah. One of the mergify rules is to have no outstanding "changes requested" reviews. Can you flip yours to "Approved"? I'll add the MWP label, and it should go in automatically once the CI is green.

[SteVwonder]

Thanks! Looks like I needed to add Mergify to the list of allowed users that can push to master, since it is a protected branch: https://docs.mergify.io/faq/?highlight=protected#mergify-is-unable-to-merge-my-pull-request-due-to-my-branch-protection-settings

Hopefully mergify re-evaluates things now and can merge.

@mergifyio refresh

[mergify[bot]]

Command refresh: success

Pull request refreshed

[dongahn]

Ha ha. That makes sense. Interesting the bot was smart enough to give her thumb up!