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.

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

chore: updating dependabot to support gha, python, docker and dev container packages #2945

Merged
merged 4 commits into from
May 24, 2024

Conversation

rajpalc7
Copy link
Contributor

No description provided.

@WadeBarnes
Copy link
Contributor

cc @ff137

@WadeBarnes
Copy link
Contributor

Looks good to me, but I'd like to get feedback from other maintainers.

@swcurran
Copy link
Contributor

@WadeBarnes — could you plan on doing a presentation on this at the next ACA-Pug — Tuesday? Just a brief summary of the impact/purpose of this and any other steps that you recommend be done. Please let me know.

@WadeBarnes
Copy link
Contributor

@WadeBarnes — could you plan on doing a presentation on this at the next ACA-Pug — Tuesday? Just a brief summary of the impact/purpose of this and any other steps that you recommend be done. Please let me know.

Sure

@ff137
Copy link
Contributor

ff137 commented May 10, 2024

I notice that some of the docker directories being maintained have Dockerfiles that look like:

ARG ELASTIC_VERSION

# https://www.docker.elastic.co/
FROM docker.elastic.co/elasticsearch/elasticsearch:${ELASTIC_VERSION}

and I'm curious if dependabot would be able to know the ELASTIC_VERSION env var, in order to determine if an update is necessary. Not sure how that would work, but can probably be tested/fixed after merge.

Also, there's of course a lot of repetition, but I think that's just part of dependabot. I don't think there's a way yet to group e.g. 1 docker update across multiple directories. So defining multiple directories is probably the way to do it.

However, there is a way to reduce repetition with YAML anchors. Mainly for the schedule, which is repeated across each group. GPT tells me it can be done, something like:

updates:
  # Define a common schedule using an anchor
  .common_schedule: &common_schedule
    schedule:
      interval: "weekly"
      day: "monday"
      time: "04:00"
      timezone: "Canada/Pacific"

  - package-ecosystem: "github-actions"
    directory: "/"
    <<: *common_schedule

  - package-ecosystem: "pip"
    directory: "/demo"
    <<: *common_schedule
    ignore:
      - dependency-name: "*"
        update-types: ["version-update:semver-major"]

Which would allow schedule to be edited in one place, and reflected everywhere. Maybe worth trying.

Lastly, for pip, I'm not sure if it makes sense to ignore major releases:

    ignore:
      - dependency-name: "*"
        update-types: ["version-update:semver-major"]

because not all major upgrades are breaking, and it's probably good to know about major releases when they come out.
What may be worth ignoring instead, are patch releases, because they have higher frequency, and are usually covered by most dependencies use "^" or "~" to take the highest compatible release. So updating the poetry lock file should include patch releases by default. Therefore dependabot can probably ignore patch releases instead, to reduce spam.

@ff137
Copy link
Contributor

ff137 commented May 10, 2024

Come to think about common schedules -- there may be a lot of dependabot PRs that open up at the same time. If integration tests are configured to run for every PR, then Monday mornings may see a lot of simultaneous integration test runs. Maybe it scales? Idk, just something to keep in mind. Using different schedules may be logical. For example, maybe pip for the main directory can be checked daily? And the demo folders checked weekly, and they open up on Saturdays. And Docker updates open on Sunday? Something like that. Just my 2c!

Also, @rajpalc7 you can check out my prev PR: #2942. There's some extra options: commit-message.prefix and labels much may be worth adding

@WadeBarnes
Copy link
Contributor

WadeBarnes commented May 10, 2024

@ff137, Thanks for the feedback. Good discussion points for the ACA-PUG meeting next week.

BTW, the dependency and ecosystem (language name) labels come for free, no need to define them.

@WadeBarnes
Copy link
Contributor

WadeBarnes commented May 14, 2024

Comments during ACA-PUG meeting:

  • Ignore patch releases, to reduce noise.
  • @dbluhm, dev dependencies should be updated as well, to ensure they keep up with the rest of the project.
  • Spread out the update schedule to distribute integration test load.

@WadeBarnes
Copy link
Contributor

The current configuration does not group dependency updates together, therefore Dependabot will open a separate PR for each update it identifies. Based on experience with other repositories the initial volume of PRs open once a PR such as this is merge is rather large. Moving forward the volume should be more maintainable.

As commented here; hyperledger/indy-vdr#271

One strategy to manage a large volume of Dependabot PRs is to use the Dependabot PRs as a guide and create a separate PR that updates all of the dependencies that can be updated without conflicts. Merging this separate PR will trigger Dependabot to reevaluate and close any PRs that are no longer relevant. This is much more efficient than dealing with each Dependabot PR individually.

@WadeBarnes
Copy link
Contributor

@ff137, We tested the YAML anchors/aliases and confirmed they are not supported by Dependabot. This file, though valid YAML, produced the following error message in Dependabot.

image

@ff137
Copy link
Contributor

ff137 commented May 15, 2024

@WadeBarnes ah, interesting. Thanks for confirming!

Copy link
Contributor

@dbluhm dbluhm left a comment

Choose a reason for hiding this comment

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

There's been a fair amount of good discussion on this. I think we've reached a point where we call it good enough and try out this config in practice and tweak as needed.

@ff137
Copy link
Contributor

ff137 commented May 21, 2024

I notice it doesn't include the suggestions mentioned: #2945 (comment)
Staggering the schedule and ignoring patches may be worth it?

Copy link

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

@swcurran swcurran merged commit dca3ef7 into openwallet-foundation:main May 24, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants