ci: Changes for dependabot to be well-behaved

[orpheuslummis]

Relevant issue(s)

Resolves #1147 #1167

Description

UPDATE: changed this PR to instead be tackling the 'dependabot problem' directly

Problem: dependabot needs a few more things to work well for the project: less PR spam, CI checks, relation between changelog and bot.

Solution:

• dependabot PRs are prefixed by bot, and their PR title length is not checked
• dependabot runs weekly
• changelog generation configuration is updated to include a bot category

addendum: I also recommend including in the release a step that updates dependencies, in case there's a last minute (security) fix. Adding this as a note to our private release process document.

Tasks

• I made sure the code is well commented, particularly hard-to-understand areas.
• I made sure the repository-held documentation is changed accordingly.
• I made sure the pull request title adheres to the conventional commit style (the subset used in the project can be found in tools/configs/chglog/config.yml).
• I made sure to discuss its limitations such as threats to validity, vulnerability to mistake and misuse, robustness to invalidation of assumptions, resource requirements, ...

How has this been tested?

CI runs

[AndrewSisley]

thought: I wonder if it is worth keeping the 60 limit for anything that isn't created by dependabot (I dont know what our detection options here are like, could perhaps allow 75 if the title contains github.com)?

[shahzadlone]

question: If we do weekly over daily, wouldn't that mean we will then get a bulk of dependabot PRs open together that we will have to deal with rather than taking care of them as they occur?

question: What if dependabot PR title is bigger than 75?

thought: IMO the commit title limit is there to serve a purpose that is also relevant to dependabot PRs. The commit title limit is a git wide convention to ensure tidy/concise titles that don't make the history/UI/etc. messy. To make the limit longer/break a git wide convention doesn't seem like the best route IMO. So far all dependabot PRs titles when I trimmed the github.com part have passed our title validation CI step.

thought: I am unsure if we should introduce a new label deps.

[orpheuslummis]

IMO weekly spam is better than daily spam.

Dependabot PR titles should skip the PR title check with this.

What alternative would be better than 'deps'? Keep in mind the list might be in the dozens of dependency updates within a major release potentially ...

[AndrewSisley]

The commit title limit is a git wide convention to ensure tidy/concise titles that don't make the history/UI/etc. messy.

I'm quite alright with us so long as we use a special prefix for it (like deps), it allows me not to care about any line/commit that starts with it, unless I am specifically trying to find a dependabot commit (in which case the I think that a longer commit title and deps prefix is probably going to be useful, not a hindrance - either for manual scans, or with something like grep).

[shahzadlone]

IMO weekly spam is better than daily spam.

Dependabot PR titles should skip the PR title check with this.

What alternative would be better than 'deps'? Keep in mind the list might be in the dozens of dependency updates within a major release potentially ...

I think deps signals a subset of what chore already represents. The main reason to have a separate label seems to be to identify bot-like / auto generated PR commits (which may or may not adhere to our entire typical github action validation). Not that we want to track when our dependencies were updated (correct me if I am wrong).

In that case, for these bot-like/autogen tools may I suggest the label bot, example bot: Bump github.com/ugorji/go/codec from 1.2.9 to 1.2.11

[shahzadlone]

There is another issue with skipping the title validation step completely. if dependabot opens a pr with invalid: title is incorrect, the entire CI will still pass.

Would suggest to change tools/scripts/validate-conventional-style.sh to be ran with an option to skip only the length limit part but do the other validations like the label is correct or not for example.

Then would ask you to also update the tests here to reflect the change and ensure the new label is valid now (after you add the new label): tools/scripts/scripts_test.sh

Can run the tests with: make test:scripts

[orpheuslummis]

IMO weekly spam is better than daily spam.
Dependabot PR titles should skip the PR title check with this.
What alternative would be better than 'deps'? Keep in mind the list might be in the dozens of dependency updates within a major release potentially ...

I think deps signals a subset of what chore already represents. The main reason to have a separate label seems to be to identify bot-like / auto generated PR commits (which may or may not adhere to our entire typical github action validation). Not that we want to track when our dependencies were updated (correct me if I am wrong).

In that case, for these bot-like/autogen tools may I suggest the label bot, example bot: Bump github.com/ugorji/go/codec from 1.2.9 to 1.2.11

It seems useful to distinguish between chores and dependency updates especially because with the use of dependabot there might be a lot of dependency updates within a major release. This way, a user can see what 'chores' were applied to the repo, and see the list of updated dependencies as two separate lists.

[shahzadlone]

todo: Suggest another way to detect that this is a dependabot title (use the a unique bot label for all bot stuff?).

Otherwise the way it currently stands if anyone opens a PR with Bump in the title it will be automatically detected as if its a dependabot PR.

[shahzadlone]

@orpheuslummis Left a todo, but will highly suggest to have a unique label to detect the type of this auto-gen bot like PR as in future if we add other bots, this we ensure their title stuff automatically also just works as long as their label is unique.

Also not sure how I feel about deps(bot) maybe others can chime in aswell? @sourcenetwork/database-team

[codecov]

Codecov Report

Merging #1165 (757626f) into develop (eb1fae4) will increase coverage by 0.05%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           develop    #1165      +/-   ##
===========================================
+ Coverage    68.53%   68.59%   +0.05%     
===========================================
  Files          180      180              
  Lines        17046    17046              
===========================================
+ Hits         11683    11692       +9     
+ Misses        4401     4394       -7     
+ Partials       962      960       -2     

see 7 files with indirect coverage changes

[shahzadlone]

Thanks for the changes, I have some small todos for you before I can give an approval =)

[shahzadlone]

LGTM, Thanks for taking the time to sort this out! Left a minor comment.

[shahzadlone]

Also heads up the PR description is somewhat outdated now.

[shahzadlone]

@orpheuslummis use rebase instead of merge next time, please.

[orpheuslummis]

@orpheuslummis use rebase instead of merge next time, please.

why?

[shahzadlone]

@orpheuslummis use rebase instead of merge next time, please.

why?

We decided to go for the rebase dev flow as a team to avoid messy history for reviewers with having develop merges into your branch

[AndrewSisley]

@orpheuslummis use rebase instead of merge next time, please.

why?

We decided to go for the rebase dev flow as a team to avoid messy history for reviewers with having develop merges into your branch

I dont think this should be forced upon anyone, as we are squashing into develop it doesnt matter outside the context of the PR. I think rebase is always cleaner and should be the 'default' go-to, but a lot of the time it doesnt matter anyway because the commits are not clean or the PR is very small.

If a large PR gets very messy commit wise it can be a royal pain to rebase anyway, and I definitely don't think we should that on anyone (such a rebase can also feel very risky to some).

IMO rebase is prefered, and to be considered 'considerate' towards reviewers who may wish to review commit-by-commit, but not mandatory.

[orpheuslummis]

Thanks for this exchange. Ideally we can have a brief update of the contributing document soon including guidelines like this and further feedback on it.