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

rec: run daily bulk test on ubicloud #15021

Merged
merged 5 commits into from
Jan 10, 2025

Conversation

omoerbeek
Copy link
Member

@omoerbeek omoerbeek commented Jan 8, 2025

Short description

Get the package from our repo (master branch).
We run both an IPv4 only test and a IPV6 enabled one.

Checklist

I have:

  • read the CONTRIBUTING.md document
  • compiled this code
  • tested this code
  • included documentation (including possible behaviour changes)
  • documented the code
  • added or modified regression test(s)
  • added or modified unit test(s)

@coveralls
Copy link

coveralls commented Jan 8, 2025

Pull Request Test Coverage Report for Build 12709832768

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 1116 unchanged lines in 27 files lost coverage.
  • Overall coverage increased (+0.06%) to 64.892%

Files with Coverage Reduction New Missed Lines %
pdns/packethandler.cc 1 72.57%
pdns/dnswriter.hh 1 76.6%
pdns/dnsdistdist/dnsdist-doh-common.cc 2 95.09%
pdns/recursordist/test-syncres_cc2.cc 3 88.91%
pdns/misc.hh 3 87.62%
pdns/tsigverifier.cc 3 77.22%
pdns/iputils.cc 3 55.91%
pdns/recursordist/recpacketcache.hh 3 89.55%
modules/gpgsqlbackend/spgsql.cc 3 67.46%
pdns/misc.cc 4 64.45%
Totals Coverage Status
Change from base Build 12679852635: 0.06%
Covered Lines: 126244
Relevant Lines: 163711

💛 - Coveralls

Copy link
Member

@romeroalx romeroalx left a comment

Choose a reason for hiding this comment

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

LGTM! Test run: https://github.com/romeroalx/pdns/actions/runs/12674038149

Do you think this job could be included in the already existing workflow misc-dailies instead of a new one? I do not have a strong opinion about this. Maybe it is because the name is somehow similar. A new workflow as is done in this PR is also fine by me.

Copy link
Member

@Habbie Habbie left a comment

Choose a reason for hiding this comment

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

one nit that I can live with. Alexis' comment about misc-dailies makes sense, if only because our total list of workflows is getting quite big.


jobs:
run-rec-bulk-test:
if: ${{ vars.REC_BULKTEST_USE_UBICLOUD == '1' }}
Copy link
Member

Choose a reason for hiding this comment

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

this is a bit misnamed. if it is set to 0, the test does not run at all, instead of running on "not ubicloud"

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll rename and move the code to misc-dailies, although we run the risk this becoming a hotchpot of unrelated workflows that only have in common that they are run daily.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, no, I just meant the var name. As for moving, I'm torn between "we have too many workflows" and "workflows are a good tool for organising things in a way that makes sense". Thinking about it, I realise another important feature of workflows is that they can be disabled/enabled from the UI. So keeping things a bit separate makes sense.

Copy link
Member Author

Choose a reason for hiding this comment

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

What would you consider a good name for the var? I'm having trouble naming things...

Copy link
Member

Choose a reason for hiding this comment

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

UBICLOUD_DAILY_REC_BULKTEST?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done. I've left the job in the misc-daily workflow. I prefer to do reorg of that some other time.

@omoerbeek omoerbeek requested review from romeroalx and Habbie January 10, 2025 08:42
Copy link
Member

@romeroalx romeroalx left a comment

Choose a reason for hiding this comment

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

Thanks!

BTW, from the conversation in the PR I thought the variable was going to be renamed: "if it is set to 0, the test does not run at all, instead of running on "not ubicloud""

@omoerbeek
Copy link
Member Author

Thanks!

BTW, from the conversation in the PR I thought the variable was going to be renamed: "if it is set to 0, the test does not run at all, instead of running on "not ubicloud""

Ah, my interpretation was Habbie was talking about the job name....

@omoerbeek omoerbeek merged commit 8868997 into PowerDNS:master Jan 10, 2025
81 checks passed
@omoerbeek omoerbeek deleted the rec-daily-bulk branch January 10, 2025 13:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants