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

system tests: refactor registry code #19106

Merged

Conversation

edsantiago
Copy link
Member

The podman-login tests have accumulated much cruft over the
years, because that's the only place where we run a local
registry, and the process was crufty: we actually start/stopped
the registry as the first & last tests of the file. Meaning,
you couldn't do 'hack/bats 150:just-one-test' because that
would skip the registry start. And just now, a completely
unrelated test has had to be shoved into the login file.

This PR revamps the whole thing, by adding a new registry helper
module that can be used anywhere. And, once the registry is
started, it just stays running until the end of tests. (This
requires BATS 1.7 or greater).

Signed-off-by: Ed Santiago [email protected]

None

@openshift-ci openshift-ci bot added release-note-none do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jul 3, 2023
@edsantiago edsantiago force-pushed the refactor_registry branch 2 times, most recently from 41a9e8d to b76dd8a Compare July 4, 2023 00:58
Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

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

LGTM

I feel slight uncomfortable to have the registry run all the time though. Pushing images etc. has side effects on other tests.

But it's a tremendous improvement. Thank you!

@vrothberg
Copy link
Member

#19092 includes this PR now ✔️

@rhatdan
Copy link
Member

rhatdan commented Jul 4, 2023

LGTM

@edsantiago
Copy link
Member Author

I feel slight uncomfortable to have the registry run all the time though. Pushing images etc. has side effects on other tests.

I would like to understand this concern better. My thinking was that there is little cost to a registry running idle, it's just another background daemon process. (This is system tests. Serial. No tests will be pushing/pulling while other tests are running). Do registries run background maintenance or other expensive tasks? Can you tell me more about your concerns?

@vrothberg
Copy link
Member

Do registries run background maintenance or other expensive tasks?

They maybe do some garbage collection and listen for incoming requests but I don´t expect this to cause noise.

Can you tell me more about your concerns?

My main concern is that tests share the same resource. So one test can influence another. For instance, pushing to this registry could break another test using the same image or searching on it.

It's a purely hypothetical scenario at the moment. I am not that worried about it now.

@edsantiago edsantiago force-pushed the refactor_registry branch from b76dd8a to bbf855d Compare July 4, 2023 13:48
@edsantiago
Copy link
Member Author

One clean CI run, no flakes. Re-pushing for confidence.

@edsantiago edsantiago force-pushed the refactor_registry branch from bbf855d to 9f570c2 Compare July 4, 2023 15:46
@edsantiago edsantiago changed the title [WIP] system tests: refactor registry code system tests: refactor registry code Jul 4, 2023
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 4, 2023
@edsantiago edsantiago force-pushed the refactor_registry branch 2 times, most recently from 7dbd182 to 54ef708 Compare July 4, 2023 20:33
The podman-login tests have accumulated much cruft over the
years, because that's the only place where we run a local
registry, and the process was crufty: we actually start/stopped
the registry as the first & last tests of the file. Meaning,
you couldn't do 'hack/bats 150:just-one-test' because that
would skip the registry start. And just now, a completely
unrelated test has had to be shoved into the login file.

This PR revamps the whole thing, by adding a new registry helper
module that can be used anywhere. And, once the registry is
started, it just stays running until the end of tests. (This
requires BATS 1.7 or greater).

Signed-off-by: Ed Santiago <[email protected]>
@edsantiago edsantiago force-pushed the refactor_registry branch from 54ef708 to ba1355b Compare July 4, 2023 21:27
@edsantiago
Copy link
Member Author

Four CI runs with no system-test flakes. I think this is good to go.

Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 5, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jul 5, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: edsantiago, vrothberg

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [edsantiago,vrothberg]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-robot openshift-merge-robot merged commit 10615e7 into containers:main Jul 5, 2023
@edsantiago edsantiago deleted the refactor_registry branch July 5, 2023 10:33
@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Oct 4, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 4, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. release-note-none
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants