-
Notifications
You must be signed in to change notification settings - Fork 765
Fully Replace Testing Farm Packit Integration with ATEX integration #14304
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
base: master
Are you sure you want to change the base?
Fully Replace Testing Farm Packit Integration with ATEX integration #14304
Conversation
|
Skipping CI for Draft Pull Request. |
aaebb2a to
b311191
Compare
ATEX Test ResultsTest artifacts have been submitted to Testing Farm. Results: View Test Results This comment was automatically generated by the ATEX workflow. |
b311191 to
7c50296
Compare
.packit.yaml
Outdated
| branch: "gh-readonly-queue/.*" | ||
|
|
||
| # when modifying this, modify also tests/tmt-plans/ | ||
| - &test-static-checks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think you can have a section like this in YAML - it's not just a template, it's a real section that can be referenced via YAML features later, but it still needs to be a valid section.
So I would just copy/paste the job, trigger and fmf_path to the remaining two sections and drop this "template". We will probably deal with rpmbuild-ctest-fedora and fedora-cis in later PRs, moving/changing them further.
Otherwise LGTM, 10 remotes (run_tests_testingfarm.py argument default) per CentOS Stream version should be fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
something like this?
downstream_package_name: scap-security-guide
upstream_package_name: scap-security-guide
specfile_path: scap-security-guide.spec
actions:
get-current-version:
- bash utils/version.sh
srpm_build_deps:
- bash
jobs:
- job: copr_build
trigger: pull_request
targets:
- fedora-all-x86_64
- job: copr_build
trigger: commit
branch: "gh-readonly-queue/.*"
targets:
- fedora-all-x86_64
- job: tests
trigger: pull_request
fmf_path: tests/tmt
identifier: /rpmbuild-ctest-fedora
tmt_plan: /plans/contest/rpmbuild-ctest-fedora$
targets:
fedora-all: {}
- job: tests
trigger: pull_request
fmf_path: tests/tmt
identifier: fedora-cis
tmt_plan: /plans/fedora-cis$
targets:
fedora-all: {}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
btw the previous format proposed in this PR seemed to work as it was a valid packit config and the tests ran correctly. I'm not sure if you are talking about some other step not seen here. But I basically followed the previous structure of the file with the templating part.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, then maybe Packit has added some check like "if the config block doesn't have identifier, ignore it", allowing templating.
But, otherwise, the original code that started with - &test-static-checks was a valid test (or rather a series of tests, everything under /static-checks), it wasn't just a template.
My point is that YAML allows to template sections (using "anchors" / "aliases") like this:
vars:
service1:
config: &service_config
env: prod
retries: 3
version: 4.8
service2:
config: *service_config
service3:
config: *service_configwhere all services receive the same config, but service1 is still a valid service, you cannot create a service_template that defines config but otherwise does nothing, and then use it for service1, service2 and service3.
So the original /static-checks code worked, because it was a valid test specification aside from being used as a template later on, but you then removed its identifier / tmt_plan / etc., resulting in an invalid test specification.
...
At least that's what I thought before I looked at the raw file and saw I misread the diff. You're right in that the code would work, because it merges the template-ish definition with /rpmbuild-ctest-fedora:
- &test-static-checks
job: tests
trigger: pull_request
fmf_path: tests/tmt
identifier: /rpmbuild-ctest-fedora
tmt_plan: /plans/contest/rpmbuild-ctest-fedora$
targets:
fedora-all: {}
- <<: *test-static-checks
identifier: fedora-cis
tmt_plan: /plans/fedora-cis$I guess I got confused by the &test-static-checks because the section doesn't have anything to do with /static-checks anymore, so the alias name doesn't make sense. Sorry.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
something like this?
Either would work, I just misunderstood your original code, sorry.
If you decide to leave it as-is, please at least rename the anchor, so it doesn't say "static checks".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay, I will rename the section
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done by amending the last commit. 8ed1f08
7c50296 to
8ed1f08
Compare
|
@ggbecker: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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-sigs/prow repository. I understand the commands that are listed here. |
| - centos-stream-8-x86_64 | ||
| - centos-stream-9-x86_64 | ||
| - centos-stream-10-x86_64 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One more thing I'm not sure - this might impact Packit RPM building as well. I'm not sure if we want to do that as the building provides at least some level of RPM-buildability-checking.
Maybe just removing these from job: tests targets below is sufficient?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe just removing these from
job: teststargetsbelow is sufficient?
I'm not sure if I understand this part. I guess if we just revert this deletion:
- centos-stream-8-x86_64
- centos-stream-9-x86_64
- centos-stream-10-x86_64
we should have the build for centos stream as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let me try that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if I understand this part. I guess if we just revert this deletion:
That's what I meant - that the existing changes already removed the centos-stream-* targets from the job: tests block, so re-adding the job: copr_build targets back (as you did) should be sufficient. 🙂
|
Otherwise the PR LGTM, thanks! |
|
As soon as we merge this we need to flip the switches on the "Required" checks list in the protected branches, to remove the packit jobs that won't exist anymore, and add the |
|
I think this can wait a bit longer until we manage to make the ATEX script to exit non-zero when there is at least one failing test. Otherwise we won't be able to gate the PR properly, it would always report pass even if a test ended with failed status. |
Those tests should now be covered by ATEX testing.
d9233f8 to
62cdb04
Compare
Description:
upstreamplan in the contest repository: add/plans/upstreamto be used by CaC/content CI RHSecurityCompliance/contest#516Rationale:
Review Hints:
workflow_runtag:Because of token potential exposures.
TODO LIST