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

Jump to bottom

Add gitlint check to automation (SOC-9559) #3504

Closed
wants to merge 1 commit into from
Closed

Add gitlint check to automation (SOC-9559) #3504

wants to merge 1 commit into from

Conversation

rsalevsky
Copy link
Contributor

No description provided.

@rsalevsky rsalevsky force-pushed the automation_gitlint branch 7 times, most recently from 1add73a to af95bdc Compare June 21, 2019 16:26
nicolasbock
nicolasbock previously approved these changes Jun 21, 2019
View reviewed changes
Copy link
Contributor

@nicolasbock nicolasbock left a comment

Choose a reason for hiding this comment

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

👍

JanZerebecki
JanZerebecki previously requested changes Jun 21, 2019
View reviewed changes
Copy link
Contributor

@JanZerebecki JanZerebecki left a comment

Choose a reason for hiding this comment

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

This has the effect of forbidding commits that are not connected to a Jira ticket. My understanding is that that is not generally wanted, especially for this repo.

@JanZerebecki JanZerebecki requested a review from dirkmueller June 21, 2019 21:07
@JanZerebecki
Copy link
Contributor

See also discussion in #2720

@rsalevsky rsalevsky force-pushed the automation_gitlint branch 2 times, most recently from a0da4bd to 8ef817e Compare June 24, 2019 11:37
@rsalevsky rsalevsky force-pushed the automation_gitlint branch 2 times, most recently from 4ac8ad4 to 7eee3b2 Compare June 24, 2019 11:55
@rsalevsky
Copy link
Contributor Author

rsalevsky commented Jun 24, 2019 •
edited
Loading

@JanZerebecki Feel free to propose a change to the gitlint config. I just want that we follow the current policy and therefore follow the rules. Not being able to follow and understand why we did what change is eating time.

@rsalevsky rsalevsky changed the title Add gitlint check to automation Add gitlint check to automation (SOC-9559) Jun 25, 2019
@dirkmueller
Copy link
Contributor

Automation repository is used for many different purposes other than what the release engineering team cares about (like for example community usecases). Enforcement.of a jira ticket for that is IMHO preventing contribution.

I do embrace the idea of useful commit message, which is why rationales need to be listed in the git commit message, rather than checking whether there is a jira ticket reference but then have no useful commit message itself and the require to look up the ticket, which might not have the right information either.

I think we need to handle this by human reviews enforcement of reasonable commit messages

dirkmueller
dirkmueller requested changes Jun 27, 2019
View reviewed changes
Copy link
Contributor

@dirkmueller dirkmueller left a comment

Choose a reason for hiding this comment

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

See previous comment

@bmwiedemann bmwiedemann mentioned this pull request Jun 27, 2019
Open
@rsalevsky
Copy link
Contributor Author

I will do some changes to this to make it more generic.

@rsalevsky rsalevsky added the wip label Jun 27, 2019
@JanZerebecki
Copy link
Contributor

It would work for this repo if you remove the groups [title-match-regex] and [author-valid-email] from the gitlint.ini. That means having a different gitlint.ini for this and the ardana repos.

@JanZerebecki
Copy link
Contributor

See also #3509 which does exactly those changes to gitlint.ini.

@rsalevsky rsalevsky force-pushed the automation_gitlint branch 3 times, most recently from 8fad9e0 to 9198d78 Compare July 3, 2019 13:57
@rsalevsky rsalevsky force-pushed the automation_gitlint branch 15 times, most recently from 0fa8f23 to ff3dde7 Compare July 4, 2019 09:07
@SUSE-Cloud SUSE-Cloud deleted a comment from stefannica Jul 4, 2019
@rsalevsky rsalevsky removed the wip label Jul 4, 2019
travis: Add gitlint check to automation
64e7b21 
Add a Gitlint check which is allowed to fail. This should help the
reviewer and author if is commit message is good. As this is non blocking
the responsibility is with the author and reviewer.
@rsalevsky rsalevsky force-pushed the automation_gitlint branch from ff3dde7 to 64e7b21 Compare July 4, 2019 09:14
@JanZerebecki JanZerebecki dismissed their stale review July 5, 2019 15:59

After #3518 I would be ok with it.

@JanZerebecki
Copy link
Contributor

I never look there when it's not failing. So I don't see any sense in its result being ignored.

dirkmueller
dirkmueller requested changes Aug 6, 2019
View reviewed changes
Copy link
Contributor

@dirkmueller dirkmueller left a comment

Choose a reason for hiding this comment

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

we're not shipping automation so the need to have a reference for maintenance does not apply here.

I really think this is going into the wrong direction, by adding more process for adding changes rather than improving the process on how we make changes (by having proper tests and reviews on them).

@rsalevsky rsalevsky closed this Aug 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dirkmueller dirkmueller dirkmueller requested changes

@nicolasbock nicolasbock nicolasbock left review comments

@flaviodsr flaviodsr Awaiting requested review from flaviodsr

@stefannica stefannica Awaiting requested review from stefannica

@JanZerebecki JanZerebecki Awaiting requested review from JanZerebecki

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@rsalevsky @JanZerebecki @dirkmueller @nicolasbock