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

test: add integration test #373

Merged
merged 1 commit into from
Mar 6, 2024
Merged

Conversation

henrywang
Copy link
Contributor

@henrywang henrywang commented Mar 4, 2024

This PR includes integration test and and CI triggered by each PR.
Integration test includes RPM build and bootc install+upgrade. bootc inside RHEL/CS9-dev container image will be replaced by new built RPM, then the bootc install and upgrade will be tested on AWS ec2 for both x86_64 and aarch64 instances. Integration test is run by TMT and Testing Farm.

TMT + Testing Farm results:

Some credentials need to be added in this repo. Anyone can help on this? Thanks.

Copy link

openshift-ci bot commented Mar 4, 2024

Hi @henrywang. Thanks for your PR.

I'm waiting for a containers 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.

@@ -0,0 +1,178 @@
#!/bin/bash
set -exuo pipefail
Copy link
Collaborator

Choose a reason for hiding this comment

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

So this is currently a copy of the code at https://gitlab.com/bootc-org/tests/bootc-workflow-test right? Can we include that as a git submodule instead, that gets updated with renovate?

Copy link
Contributor Author

@henrywang henrywang Mar 4, 2024

Choose a reason for hiding this comment

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

Not exactly, Two reasons I decided to add a new file here:

  1. I'd like to add more test in the future, like bootc arguments tests.
  2. New feature or bug fix PR can update and run test in the same PR. And I'd like to add git submodule in bootc-workflow-test repo in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cgwalters Could you please add me into org and give me permission to add some secret, like AWS keys, etc. Thanks.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@henrywang I sent you an invite

Copy link
Collaborator

Choose a reason for hiding this comment

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

New feature or bug fix PR can update and run test in the same PR. And I'd like to add git submodule in bootc-workflow-test repo in the future.

IOW you want to have bootc-workflow-test consume this repository as a git submodule? I think that makes sense...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IOW you want to have bootc-workflow-test consume this repository as a git submodule? I think that makes sense...

Yes, upstream always first.

The workflow I would like to do:

  1. most of test code will be saved in upstream repo and QE spend most of their efforts on PRs of upstream repo.
  2. bootc-workflow-test (it will be bootc and bootc image downstream test repo) will consume upstream repo as git submodule. The git update will be triggered by new bootc RHEL brew build. That will avoid code gap between upstream(always latest) and downstream. I know it can't 100% fix the gap issue, but
  3. then downstream will run daily regular test.

@cgwalters
Copy link
Collaborator

Looks like a syntax error?

Error: The template is not valid. .github/workflows/integration.yml (Line: 42, Col: 12): Error reading JToken from JsonReader. Path '', line 0, position 0.,.github/workflows/integration.yml (Line: 43, Col: 12): Error reading JToken from JsonReader. Path '', line 0, position 0.,.github/workflows/integration.yml (Line: 44, Col: 17): Error reading JToken

@henrywang henrywang force-pushed the mock_build branch 4 times, most recently from eced96d to 860c43f Compare March 5, 2024 00:47
@henrywang
Copy link
Contributor Author

Looks like a syntax error?

Error: The template is not valid. .github/workflows/integration.yml (Line: 42, Col: 12): Error reading JToken from JsonReader. Path '', line 0, position 0.,.github/workflows/integration.yml (Line: 43, Col: 12): Error reading JToken from JsonReader. Path '', line 0, position 0.,.github/workflows/integration.yml (Line: 44, Col: 17): Error reading JToken

GITHUB_TOKEN permission issue fixed according to https://docs.github.com/en/rest/pulls/pulls?apiVersion=2022-11-28#get-a-pull-request.

@cgwalters May I have permission to add some github secrets? After secrets added, permission can be removed. Thanks!

@cgwalters
Copy link
Collaborator

@cgwalters May I have permission to add some github secrets? After secrets added, permission can be removed. Thanks!

Backing up a level...this seems like something that may even be better done at an organization level - if we ended up wanting to share testing farm bits across other repositories. cc @cevich

@cevich
Copy link
Member

cevich commented Mar 5, 2024

I'm not opposed in principal. My concern is over security, and I'm not 100% on how/where/when github action secrets are exposed. Since it would expose every org. secret to every repo., is it possible someone could craft a repo-level workflow that would leak a secret? In other words, I speculate (not verified) there are some containers-org repos. that RH isn't 100% in control over.

@cgwalters
Copy link
Collaborator

Since it would expose every org. secret to every repo., is it possible someone could craft a repo-level workflow that would leak a secret? In other words, I speculate (not verified) there are some containers-org repos. that RH isn't 100% in control over.

Yes, this is why any repository with secrets needs to carefully watch PRs that affect workflows. It's difficult and error prone. The Testing Farm bits currently require manual approval.

But my question wasn't about adding these secrets org wide, my question was more about a github.com/containers SOP (or even, a SOP for a subset of them) - does the Cirrus setup involve any repo secrets?

@cevich
Copy link
Member

cevich commented Mar 5, 2024

does the Cirrus setup involve any repo secrets?

Cirrus-CI supports both, org-level and repo-level. The org-level secrets are dangerous for the same reason as I outlined above. The repo-level secrets are only as safe as the (per-repo) "anybody can decrypt" setting. When that's turned off, it has similar "require approval" button behavior. That became a MAJOR headache on the high-traffic repos, so we keep it turned on. That makes org-wide Cirrus-CI secrets even more dangerous.

So my SOP is to avoid anything but very inconsequential secrets in cirrus, except for repos. where that "anybody can decrypt" button can be left/turned off. Otherwise, for high-traffic repos, I'd prefer to keep all secret stuffs nestled away in the github settings. or other safe places.

Minor Edit/Note: The ENCRYPTED[abc123] strings in the .cirrus.yml are actually database indexes. The actual secrets are locked to the repo. they were generated for. Forks cannot use or decrypt them. Cirrus-CI does support OIDC and Hashi-Vault, but the setup is complex enough I've not contemplated it yet.

@cgwalters
Copy link
Collaborator

/ok-to-test

@henrywang
Copy link
Contributor Author

@cgwalters Running testing farm needs TF_API_KEY secret available inside the forked repo. So the pull_request_target trigger has to be used in this case. To protect the secrets, this workflow has a PR sender permission checking at first job. Only collaborator with repo write or admin permission can run this workflow.

pull_request_target only runs in the pull request's target branch, and not the pull request's branch itself. This PR can't trigger this workflow included in this PR.

Signed-off-by: Xiaofeng Wang <henrywangxf@me.com>

on:
pull_request_target:
types: [opened, synchronize, reopened]
Copy link
Collaborator

Choose a reason for hiding this comment

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

I noticed the test isn't actually running on this PR...did you test this manually in some way?

Or do we need to merge and then test on a new PR?

Copy link
Contributor Author

@henrywang henrywang Mar 6, 2024

Choose a reason for hiding this comment

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

Or do we need to merge and then test on a new PR?

Correct. pull_request_target only runs in the pull request's target branch, and not the pull request's branch itself. This PR can't trigger this workflow included in this PR. Merge this PR first, then I'll send another PR to test CI.
BTW: I've run the test code locally with TMT+Testing Farm. Test code part works.

@cgwalters cgwalters merged commit c590788 into containers:main Mar 6, 2024
11 checks passed
@henrywang henrywang deleted the mock_build branch March 6, 2024 15:21
@cevich
Copy link
Member

cevich commented Mar 6, 2024

@cgwalters one more thing I forgot that may be helpful: Above I was talking about environment secrets (the ones that show up as envars) for scripts to use. Any secrets used by Cirrus-CI (the service) to access clouds, worker pools, or kube clusters are treated differently. They cannot leak into any script environment, and are well protected. Though there's no reason the underlying account can't be shared, the actual ENCRYPTED[abc123] values still must be generated specifically for each repo, one by one. Hope that helps.

@cgwalters
Copy link
Collaborator

ISTM that testing farm could do the same thing here by being a proper app instead of a raw action? I guess to say this another way I know there's been an investment in Cirrus (and Prow, and GH actions) on other...wait, I see podman is running testing farm tests, but it looks like indirectly via packit? Is that the better flow?

@cevich
Copy link
Member

cevich commented Mar 6, 2024

Careful, don't fall into the sunk-cost fallacy 😄 I think the intended way to do this is to have both a pull_request_target and pull_request workflow call into a reusable workflow to run the tests. That gives the best of both worlds, no?

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.

None yet

3 participants