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

fix: continue on no update for repository #273

Merged
merged 2 commits into from
Nov 22, 2024

Conversation

jmrt47
Copy link
Contributor

@jmrt47 jmrt47 commented Nov 22, 2024

Pull Request

Proposed Changes

In #265 a bug was introduced by removing the continue statement which prevents further processing of issue/pr creation for repositories where no update to the dependabot file was found. This PR just adds the continue statement again. Bug was confirmed by @ricardojdsilva87 already in comment of #265 and a fix is also part of #270.

Right now all evergreen runs fail with an error as soon as the first repository without changes is processed.

Readiness Checklist

Author/Contributor

  • If documentation is needed for this change, has that been included in this pull request
  • run make lint and fix any issues that you have introduced
  • run make test and ensure you have test coverage for the lines you are introducing
  • If publishing new data to the public (scorecards, security scan results, code quality results, live dashboards, etc.), please request review from @jeffrey-luszcz

Reviewer

  • Label as either fix, documentation, enhancement, infrastructure, maintenance or breaking

@jmeridth
Copy link
Member

@jmrt47 thank you for the fix. Is a test possible? Wdyt?

@jmrt47
Copy link
Contributor Author

jmrt47 commented Nov 22, 2024

@jmrt47 thank you for the fix. Is a test possible? Wdyt?

@jmeridth I thought of introducing a test as well, but currently it seems a bit too complicated for me to test the main method as a whole.

IMO the main method should be refactored and chopped into chunks which are easy to understand by follow one purpose. This way the logic of the method could also easier be tested.

@jmeridth
Copy link
Member

@jmrt47 thank you for the fix. Is a test possible? Wdyt?

@jmeridth I thought of introducing a test as well, but currently it seems a bit too complicated for me to test the main method as a whole.

IMO the main method should be refactored and chopped into chunks which are easy to understand by follow one purpose. This way the logic of the method could also easier be tested.

Agreed. I'm good merging this with a test in mind. Can you file an issue for it please so we don't lose track of it?

@jmeridth jmeridth merged commit c66dac6 into github:main Nov 22, 2024
6 checks passed
@jmrt47
Copy link
Contributor Author

jmrt47 commented Nov 22, 2024

@jmrt47 thank you for the fix. Is a test possible? Wdyt?

@jmeridth I thought of introducing a test as well, but currently it seems a bit too complicated for me to test the main method as a whole.
IMO the main method should be refactored and chopped into chunks which are easy to understand by follow one purpose. This way the logic of the method could also easier be tested.

Agreed. I'm good merging this with a test in mind. Can you file an issue for it please so we don't lose track of it?

Thanks, I put the idea in issue #275 to not lose track

@jmrt47 jmrt47 deleted the continue-on-no-update branch November 22, 2024 08:15
@jmeridth
Copy link
Member

Whoops. Looks like #270 may have fixed the issue also (from original author). Looking at that now.

@ricardojdsilva87
Copy link
Contributor

@jmeridth yes indeed I have opened #270 right after the previous release that enabled the private repositories feature since mistakenly I had removed the continue from the code.
I have also added a few more tests and features (missing package managers), feel free to check it.
Thanks

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.

3 participants