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.

Sign up for GitHub

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

gitlint.ini improvements #3509

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

gitlint.ini improvements #3509

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
16e63ad
gitlint.ini: Allow external contributions
bmwiedemann Jun 27, 2019
5a238f5
gitlint.ini: Do not require a issue reference
bmwiedemann Jun 27, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 0 additions & 6 deletions scripts/jenkins/gitlint.ini
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,14 +7,8 @@ line-length=80
[title-must-not-contain-word]
words=wip,WIP

[title-match-regex]
regex=^(?!SOC).*\(((bsc#|SOC-)[0-9]+(,\s)?)+\)$
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of removing it we should improve this:

  • not reference for simple commits e.g. via a keyword?
  • if there is a reference then it should be formatted correctly
  • this should be required for Ardana and Crowbar
  • do we want to keep/extend the subject category's .e.g "mkcloud: Add SLES 12 ... support"

Copy link
Member Author

Choose a reason for hiding this comment

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

So we could allow for a "trivial" prefix, but what about the non-trivial ones? What is the advantage of making people open a bug for these?
If there is already a related bug, it probably is a good idea to reference it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would tend to say non-trivial are tacking longer and therefore have to be planned as part of the agile team setup. In such a case we will automatically have a Jira ticked.

+1 for the Trivial prefix

Copy link
Contributor

Choose a reason for hiding this comment

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

@rsalevsky Do you think there is room between trivial (e.g. a typo) and the smallest thing that needs to be planned?

As you mentioned agile, I think requiring tickets for everything is the opposite of 2 of the value trade-offs from the http://agilemanifesto.org/ :

Individuals and interactions over processes and tools

Responding to change over following a plan

Copy link
Contributor

Choose a reason for hiding this comment

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

@rsalevsky By the time there is a review the work has already been done, so no more planning is needed.

I understand the concern that if there is a jira ticket for some change then it should be referenced. but enforcing to create a jira ticket just in order to fix something or cleanup the code makes not a lot of sense to me. We shouldn't bog down in process but make it easy to fix the code. If I need to first wait two weeks for the next sprint planning in order to fix something then that feels very cumbersome.

Given that we're currently not delivering this repository as mainteannce update, what is the value of enforcing bug and jira references? I see some value when we're enforcing bug references for customer visible changes, but this isn't of that kind.

Copy link
Contributor

Choose a reason for hiding this comment

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

@rsalevsky I'm not against adding refences to jira/bugzillas when they exist, they totally should be included. Tracking the QA state is done for that purpuse in ardana via the separate "QE-Review" workflow.

Regarding your first work, just tracking that there was one pr merged does not mean the jira has made "significant progress", it really depends. There could be more than one review need to be merged for a particular user story to be compled, so just because there was a PR merged does not indicate where the story is done or not.

IMHO the story is done when the acceptance criteria is fulfilled, and the acceptance criteria should specify what needs to be done. I actually don't see value in people moving tickets in their state just because they saw some PR being merged somewhere - only the "worker" (assignee) should be in charge of doing so, and its the scrum master's responsibility to remind the worker to proprely track the status.

There could be e.g. upstream reviews needed for a particular user story. are also going to ask u pstream to accept our internal jira ticket references in the commit messages? Certainly not. So why here?

Copy link
Contributor

Choose a reason for hiding this comment

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

@rsalevsky I'm not against adding refences to jira/bugzillas when they exist, they totally should be included. Tracking the QA state is done for that purpuse in ardana via the separate "QE-Review" workflow.

And this works right? ;)

Regarding your first work, just tracking that there was one pr merged does not mean the jira has made "significant progress", it really depends. There could be more than one review need to be merged for a particular user story to be compled, so just because there was a PR merged does not indicate where the story is done or not.

Agreed but I never said the opposite. It's really depends case by case. But it prevents that I have to ask where the code is (even we the story is done) as it is linked automatically. Plus if the code is merged since weeks and the card did not move it's a indicator that something is not correct about the status.

IMHO the story is done when the acceptance criteria is fulfilled, and the acceptance criteria should specify what needs to be done. I actually don't see value in people moving tickets in their state just because they saw some PR being merged somewhere - only the "worker" (assignee) should be in charge of doing so, and its the scrum master's responsibility to remind the worker to proprely track the status.

Agreed but what is if this doesn't happen? Knowing as much as possible about a story helps Project Management to determine the health of it.

There could be e.g. upstream reviews needed for a particular user story. are also going to ask u pstream to accept our internal jira ticket references in the commit messages? Certainly not. So why here?

What has upstream to do with Crowbar and Ardana? I'm not getting this point.

Copy link
Contributor

Choose a reason for hiding this comment

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

Once you revisit a ticket you will find your PR and can add it as a comment to the ticket. You'd need to do the same thing for upstream PRs that don't accept our internal ticket references. If you like those also in git you could use git notes for that. Yes that is more work than not forgetting it, but so is amending the PR.

However if nobody ever revisits the ticket e.g. because the product is now out of support and has no new version, then it ceased to matter anyway. Having the ticket linked to the PR doesn't help with that either way. If you have something else with "what if this doesn't happen" in mind, maybe we can chat about it with some concrete examples.

Copy link
Contributor

Choose a reason for hiding this comment

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

@JanZerebecki Let's forget upstream as this is handled different in the MU process. For each Crowbar update I spend 3 hours to try to match Jira and Crowbar Commit messages each week. This are minimum 20 commits. So I have to revisit every ticked and every commit. ;) I don't think it is fair that $someone has to do this stupid works just because we are not enforcing our own policy?

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you ever do that for automation.git?


[body-max-line-length]
line-length=80

[body-min-length]
min-length=5

[author-valid-email]
regex=[^@]+@suse.(com|de)