-
Notifications
You must be signed in to change notification settings - Fork 9
Migrating to GH issues #166
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
base: main
Are you sure you want to change the base?
Conversation
CONTRIBUTING.md
Outdated
|
|
||
| - *Original and Remaining Estimate* for estimating resources | ||
| - *Estimated Difficulty* for estimating complexity | ||
| Don't forget to indicate which version of Narayana LRA, Java and Maven you are using. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@marcosgopen maybe we can incorporate some of this in an issue template (https://docs.github.com/en/communities/using-templates-to-encourage-useful-issues-and-pull-requests/configuring-issue-templates-for-your-repository)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the idea, I will create an issue template.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I opened a separate PR for it: #168
CONTRIBUTING.md
Outdated
| If the change would result in behaviour in Narayana LRA that is incompatible with the current release stream for Narayana LRA, then please specify it in the issue description. | ||
| If the change would result in behaviour in Narayana that is incompatible with the current release stream for Narayana, then the relevant JIRA issue `Fix Version/s` must be set to the `<version+1>.next`. For example if the root pom.xml is currently `<version>7.0.3.Final-SNAPSHOT</version>` then the JIRA issue should have a `Fix Version/s` of `8.next` added. When resolving the issue in JIRA we encourage the assignee to consider adding a release note describing the impact of the change. | ||
| When the github Pull Request has passed all relevant GitHub action checks and has been Approved by a reviewer the code can be merged. If you don't have permission to do this then ping one of the team who will then merge it. Once merged the issue must be updated in the issue tracker (if you don't have permission then a team member will do this): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there are some text "casing" issues in this line. github in lower case and Approved being capitalised
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 thanks
| fi | ||
| } | ||
|
|
||
| function check_if_pull_closed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I amn not super certain but I think this change is unrelated to the broader issue being resolved here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I will create a separate commit for that.
|
|
||
| function enable_as_trace { | ||
| CONF=${1:-"${JBOSS_HOME}/standalone/configuration/standalone-xts.xml"} | ||
| CONF=${1:-"${JBOSS_HOME}/standalone/configuration/standalone.xml"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, I think this should be a separate commit as Tom said above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I will create a separate commit for that.
CONTRIBUTING.md
Outdated
| This includes bug reports, fixes, documentation, examples... | ||
|
|
||
| If you are looking for an issue to work on and haven't had much previous experience with Narayana then you could choose one that has the label [good-first-issue or hacktoberfest](https://issues.redhat.com/browse/JBTM-2493?filter=12421681) or one whose "Estimated Difficulty" field is `Low`. For Hacktoberfest we have created a zulip stream called [hacktoberfest](https://narayana.zulipchat.com/#narrow/stream/406889-hacktoberfest/topic/stream.20events/near/393204842) for discussions. If you want to take an issue then ping a team member (or add a message to the zulip stream) and we will update the assignee field. | ||
| If you are looking for an issue to work on and haven't had much previous experience with Narayana LRA then you could choose one that has the label [good-first-issue or hacktoberfest](https://github.com/jbosstm/lra/issues?q=is%3Aissue%20state%3Aopen%20label%3A%22good%20first%20issue%22%20OR%20label%3Ahacktoberfest). For Hacktoberfest we have created a zulip stream called [hacktoberfest](https://narayana.zulipchat.com/#narrow/stream/406889-hacktoberfest/topic/stream.20events/near/393204842) for discussions. If you want to take an issue then ping a team member (or add a message to the zulip stream) and we will update the assignee field. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you really want the hacktoberfest issue in this link? I don't think it's a good idea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, good first issue label is enough.
CONTRIBUTING.md
Outdated
| * Clean up changes not being in a separate git commit | ||
| * Commit message not prefixed with a JIRA number | ||
| * Too many separate commits in the same pull request related to the same JIRA and the same part of the change | ||
| * Too many separate commits in the same pull request related to the same GitHub issue and the same part of the change |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can move to squash merging if you prefer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that is a good idea. I will update the merging strategy and remove this line.
0213779 to
7e5d291
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @marcosgopen
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving, but consider removing that one point.
| 6. The engineer raising the PR should have tested their changes prior to submitting the request to merge changes. However, there are some circumstances where this may not be possible in which case you must add the label "Hold" and update the PR description indicating why it isn't ready for review just yet. The policy to add the label "Hold" is a signal to reviewers that the changes are not yet ready to be reviewed. | ||
| 1. The Pull Request *should* contain a link to the GitHub issue(s) at the start of the PR description (only minor changes to script/text files are exempt from this rule). If the engineer wishes to address multiple issues and they are closely related then they can be addressed in a single PR. The GitHub issue must contain sufficient information to enable the reader to understand what the issue is, so at a minimum the description field of the GitHub issue must be present and legible/clear. | ||
| 2. If the Pull Request depends on other pull requests, please put the full URL of the pull request(s) that you depend on. Ideally with a prefix "Depends on ". | ||
| 3. Engineers are not allowed to submit PRs which only contain formatting changes. The guidance on formatting code are covered in the [Coding Guidelines](#coding-guidelines) section below. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this was here before but it kind of peeked my interest. I don't see coding-guidelines section in this document. Also, I think why not? Let people open PRs that will improve things. Doesn't matter how big the improment might be.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well spotted @xstefank, the coding guidelines is above, not below. I will fix that.
Concerning your comment "Let people open PRs that will improve things. Doesn't matter how big the improment might be.", do you refer to the sentence "If the engineer wishes to address multiple issues and they are closely related then they can be addressed in a single PR."? If so I could rephrase that point to something like:
"The Pull Request should contain a link to the GitHub issue(s) at the start of the PR description (only minor changes to script/text files are exempt from this rule). If the contributor wishes to address multiple issues in a single PR they are encouraged to make separate commits and add a description for each commit if needed. The GitHub issue must contain sufficient information to enable the reader to understand what the issue is, so at a minimum the description field of the GitHub issue must be present and legible/clear."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I just meant that we should also allow formatting code changes if they improve the code. Let us decide on the PR, not write this into guidelines.
Fixes #72