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.

Sign up for GitHub

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

Jump to bottom

OPCT-28: Support Disconnected Mirror Registry #45

Merged
merged 3 commits into from
Mar 25, 2023
Merged

OPCT-28: Support Disconnected Mirror Registry #45

merged 3 commits into from
Mar 25, 2023

Conversation

rvanderp3
Copy link
Contributor

@rvanderp3 rvanderp3 commented Feb 6, 2023 •
edited by mtulio

Introduces a new argument to the run command called --image-repository . When provided the --image-repository argument, the tool will look to the provided registry as the source of images.

Plugin updates: redhat-openshift-ecosystem/provider-certification-plugins#37

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 6, 2023
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 7, 2023
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 14, 2023
@rvanderp3 rvanderp3 marked this pull request as ready for review February 14, 2023 21:01
@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 Feb 14, 2023
@openshift-ci openshift-ci bot requested review from bostrt and mtulio February 14, 2023 21:01
@rvanderp3 rvanderp3 force-pushed the OPCT-28 branch 2 times, most recently from fe9453a to 4eb133b Compare February 14, 2023 21:04
@rvanderp3
Copy link
Contributor Author

/assign @mtulio

@rvanderp3 rvanderp3 changed the title OPCT-28: [wip] introduce image-repository argument OPCT-28: Support Disconnected Mirror Registry Feb 14, 2023
@mtulio
Copy link
Contributor

mtulio commented Feb 14, 2023 •
edited

Nit's from CI, but overall looks great. I will take a second look tomorrow at the plugin's change!
/hold

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 14, 2023
@mtulio mtulio mentioned this pull request Feb 15, 2023
Merged
mtulio
mtulio reviewed Feb 15, 2023
View reviewed changes
Copy link
Contributor

@mtulio mtulio left a comment

Choose a reason for hiding this comment

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

Great! Some suggestions and fixes for CI issues.

I am also thinking about how we can run the upgrade mode in disconnected environments?

docs/user-installation-disconnected.md Show resolved Hide resolved
docs/user-installation-disconnected.md Outdated Show resolved Hide resolved
manifests/openshift-conformance-validated.yaml Show resolved Hide resolved
pkg/run/run.go Outdated Show resolved Hide resolved
pkg/run/run.go Outdated Show resolved Hide resolved
pkg/run/run.go Show resolved Hide resolved
@rvanderp3 rvanderp3 force-pushed the OPCT-28 branch 2 times, most recently from 73ebab2 to 28d1cb0 Compare February 15, 2023 16:11
mtulio
mtulio reviewed Feb 16, 2023
View reviewed changes
docs/user-installation-disconnected.md Outdated
Comment on lines 54 to 56
## Running Tests with the Disconnected Mirror registry

To run tests such that they use images hosted by the Disconnected Mirror registry:

~~~
./openshift-provider-cert-linux-amd64 run --image-repository ${TARGET_REPO}
~~~

For additional details and configuration options, see [OpenShift Provider Certification Tool - User Guide](./user.md).
Copy link
Contributor

Choose a reason for hiding this comment

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

@rvanderp3 sorry I missed that in my last review:
Our User Guide was structured sequentially. So, the section "Usage/Run" aims to describe each execution mode (regular, upgrade) and now disconnected. What do you think about moving this part (run) to that section?
Or feel free to suggest if it current way is confusing.

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 16, 2023
@rvanderp3
Copy link
Contributor Author

Great! Some suggestions and fixes for CI issues.

I am also thinking about how we can run the upgrade mode in disconnected environments?

i had not thought about that. let's tackle that in a follow on PR

@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 6, 2023
mtulio
mtulio reviewed Mar 21, 2023
View reviewed changes
Copy link
Contributor

@mtulio mtulio left a comment

Choose a reason for hiding this comment

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

Overall LGTM after updating the image ref and generating it on plugins repo.
If you want we can address the disconnected upgrade in another PR.

docs/user-installation-disconnected.md Outdated Show resolved Hide resolved
pkg/types.go Outdated Show resolved Hide resolved
docs/user-installation-disconnected.md Outdated Show resolved Hide resolved
@rvanderp3 @mtulio
Apply suggestions from code review
f758870 
Co-authored-by: Marco Braga <braga@mtulio.eng.br>
@rvanderp3
Copy link
Contributor Author

/hold cancel

/assign @mtulio

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 23, 2023
@mtulio
Copy link
Contributor

mtulio commented Mar 25, 2023 •
edited

Thanks! o/
/lgtm
/approve

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

openshift-ci bot commented Mar 25, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mtulio

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:

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 25, 2023
@openshift-merge-robot openshift-merge-robot merged commit 4d3aff2 into redhat-openshift-ecosystem:main Mar 25, 2023
openshift-merge-robot pushed a commit to redhat-openshift-ecosystem/provider-certification-plugins that referenced this pull request Mar 27, 2023
@rvanderp3
OPCT-28: Introduce mirror repository variable (#37)
993b6c7 
Relates to
redhat-openshift-ecosystem/provider-certification-tool#45
@mtulio mtulio added the kind/feature Categorizes issue or PR as related to a new feature. label Apr 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mtulio mtulio mtulio left review comments

@bostrt bostrt Awaiting requested review from bostrt

Assignees

@mtulio mtulio

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/feature Categorizes issue or PR as related to a new feature. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@rvanderp3 @mtulio @openshift-merge-robot