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

Included Dependabot #2980

Merged
merged 1 commit into from
Oct 27, 2021
Merged

Conversation

nathannaveen
Copy link
Contributor

@nathannaveen nathannaveen commented Oct 26, 2021

What changes were proposed in this pull request?

Enable dependabot to get security updates and if needed version updates on dependencies

Why are the changes needed?

https://docs.github.com/en/code-security/supply-chain-security/keeping-your-dependencies-updated-automatically

Having knowledge about vulnerabilities of the dependencies helps the project owners decide on their dependency's security posture to make decisions.

If the project decides to get updates only on security updates and not on any version updates then setting these options would not open any PR's open-pull-requests-limit: 0

This option has to be enabled in the security section of the project.
https://docs.github.com/en/code-security/supply-chain-security/managing-vulnerabilities-in-your-projects-dependencies/configuring-dependabot-security-updates#managing-dependabot-security-updates-for-your-repositories

This is a recommendation from OSSF: https://github.com/ossf/scorecard/blob/main/docs/checks.md#dependency-update-tool which is part of the Linux Foundation.

@k8s-ci-robot
Copy link
Collaborator

Hi @nathan-415. Thanks for your PR.

I'm waiting for a google member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Copy link
Contributor

@kolyshkin kolyshkin left a comment

Choose a reason for hiding this comment

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

In this repo, cmd directory has separate go.mod and go.sum and thus it also needs an entry in dependabot.yml.

.github/dependabot.yml Outdated Show resolved Hide resolved
.github/dependabot.yml Outdated Show resolved Hide resolved
@kolyshkin
Copy link
Contributor

In addition, there are a few Dockefiles under deploy, and they might benefit from having package-ecosystem: "docker" section in dependabot config.

Enable dependabot to get security updates and if needed version updates on dependencies.
@nathannaveen
Copy link
Contributor Author

In this repo, cmd directory has separate go.mod and go.sum and thus it also needs an entry in dependabot.yml.

I have included for cmd and docker. Thanks.

@nathannaveen
Copy link
Contributor Author

@kolyshkin I have made the changes you have requested.

@bobbypage
Copy link
Collaborator

/ok-to-test

Copy link
Contributor

@kolyshkin kolyshkin left a comment

Choose a reason for hiding this comment

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

LGTM(nb), thank you @nathan-415!

@bobbypage
Copy link
Collaborator

looks good, thanks @nathan-415 !

@bobbypage bobbypage merged commit 14155ee into google:master Oct 27, 2021
@nathannaveen nathannaveen deleted the nathan/dependabot branch October 28, 2021 00:00
@nathannaveen
Copy link
Contributor Author

LGTM(nb), thank you @nathan-415!

I understand what LGTM, but what is “nb"?

@bobbypage
Copy link
Collaborator

"non binding" I think :)

@kolyshkin
Copy link
Contributor

Oh well, this opened a gates of hell.

Problem is, dependabot updates go deps for main module and for ./cmd separately, and this breaks CI for both PRs.

Unless there's some sort of an option to do this at once, it's not going to work (except as a reminder to update).

@bobbypage
Copy link
Collaborator

bobbypage commented Oct 28, 2021

Yes, it sounds like dependabot might be more trouble that it's worth. In addition to the issue of update of ./cmd and ./ main module being separated and breaking CI, there's another issue that since cAdvisor is vendored into k/k and dependency updates in cAdvisor will have to make it back in k/k.

k/k sometimes sticks to older versions of dependencies for various reasons/blockers, so to avoid churn when vendoring cAdvisor back into k/k I think we should stick with manual updates.

Github will still alert for any dependencies with security issues, so we should be made aware of out of date deps with security issues.

As a result of those two reasons, let's revert dependabot for now.

@nathannaveen
Copy link
Contributor Author

Yes, it sounds like dependabot might be more trouble that it's worth. In addition to the issue of update of ./cmd and ./ main module being separated and breaking CI, there's another issue that since cAdvisor is vendored into k/k and dependency updates in cAdvisor will have to make it back in k/k.

k/k sometimes sticks to older versions of dependencies for various reasons/blockers, so to avoid churn when vendoring cAdvisor back into k/k I think we should stick with manual updates.

Github will still alert for any dependencies with security issues, so we should be made aware of out of date deps with security issues.

As a result of those two reasons, let's revert dependabot for now.

Thanks, makes sense. I hope you have enabled the Dependabot settings to get security updates.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants