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

Use github.com/openshift/runtime-utils/pkg/registries #205

Merged
merged 2 commits into from
Apr 28, 2022
Merged

Use github.com/openshift/runtime-utils/pkg/registries #205

merged 2 commits into from
Apr 28, 2022

Conversation

mtrmac
Copy link
Contributor

@mtrmac mtrmac commented Apr 26, 2021 •
edited
Loading

... instead of a local copy.

The code is currently 100% identical, so this doesn't make a difference, but that will change with openshift/runtime-utils#11 .

Fixes #41 .

@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: mtrmac
To complete the pull request process, please assign sttts after the PR has been reviewed.
You can assign the PR to them by writing /assign @sttts in a comment when ready.

The full list of commands accepted by this bot can be found 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

@mtrmac
Copy link
Contributor Author

mtrmac commented Apr 26, 2021

@umohnani8 FYI. The wildcard support will also require changes to pkg/image/apiserver/importer.ImageStreamImporter .

@mtrmac mtrmac mentioned this pull request Apr 26, 2021
Closed
@openshift-ci openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 3, 2021
@openshift-ci openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 3, 2021
@openshift-ci openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 30, 2021
@openshift-ci openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 31, 2021
@mtrmac
Copy link
Contributor Author

mtrmac commented Jun 2, 2021

/retest

@openshift-ci openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 14, 2021
@openshift-ci openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 14, 2021
@mtrmac
Copy link
Contributor Author

mtrmac commented Jul 15, 2021

/retest

@umohnani8
Copy link

umohnani8 commented Aug 2, 2021 •
edited
Loading

@mtrmac do I need to wait for this PR to get in before making the changes suggested above in #205 (comment)?

@openshift-ci openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 2, 2021
@mtrmac
Copy link
Contributor Author

mtrmac commented Aug 2, 2021

@mtrmac do I need to wait for this PR to get in before making the changes suggested above in #205 (comment)?

@umohnani8 It would be a start, but it needs to be followed by updating to a version that supports wildcards; only then can ImageStreamImporter be extended to read wildcard configuration. So, alternatively, it would be possible to skip this code-move-only PR and directly vendor a version with wildcards.

@openshift-ci openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 2, 2021
@mtrmac
Copy link
Contributor Author

mtrmac commented Aug 3, 2021

/retest

1 similar comment
@mtrmac
Copy link
Contributor Author

mtrmac commented Aug 5, 2021

/retest

@openshift-bot
Copy link
Contributor

Issues go stale after 90d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle stale

@openshift-ci openshift-ci bot added lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Nov 3, 2021
@openshift-ci openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 3, 2021
@openshift-bot
Copy link
Contributor

Stale issues rot after 30d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle rotten
/remove-lifecycle stale

@openshift-ci openshift-ci bot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Dec 3, 2021
@openshift-ci openshift-ci bot added lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Mar 4, 2022
@mtrmac
Copy link
Contributor Author

mtrmac commented Mar 4, 2022

Almost a year later… @QiWang19 this is one of the repos that should eventually be updated for openshift/enhancements#929 .

@mtrmac
Use github.com/openshift/runtime-utils/pkg/registries
438daac 
... instead of a local copy.

The code is currently 100% identical, so this doesn't
make a difference, but that will change with
openshift/runtime-utils#11 .

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
@mtrmac
Update github.com/openshift/runtime-utils
489928c 
$ go get -u github.com/openshift/runtime-utils
$ go mod tidy
$ go mod vendor

+ updates to use c/image/v5 instead of the old 3.0.2 version.

Note that the various direct uses of sysregistriesv2, e.g.
pkg/image/apiserver/importer/importer.go , are pretty likely to
be obsolete/incorrect by now.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
@openshift-ci openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 4, 2022
@mtrmac
Copy link
Contributor Author

mtrmac commented Mar 4, 2022

I have rebased the original ~do-nothing commit, and added one that updates to the current version of runtime-utils. Note that there is some code that interprets the config directly, e.g. in pkg/image/apiserver/importer/importer.go and that looks obsolete/broken with the new semantics. So more work would be necessary to fully support non-registry scopes.

@mtrmac
Copy link
Contributor Author

mtrmac commented Mar 4, 2022

/retest

1 similar comment
@mtrmac
Copy link
Contributor Author

mtrmac commented Mar 7, 2022

/retest

@openshift-bot
Copy link
Contributor

Stale issues rot after 30d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle rotten
/remove-lifecycle stale

@openshift-ci openshift-ci bot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Apr 6, 2022
@umohnani8
Copy link

/remove-lifecycle rotten
@mtrmac is there anything that is blocking this from getting in?

@openshift-ci openshift-ci bot removed the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label Apr 14, 2022
@mtrmac
Copy link
Contributor Author

mtrmac commented Apr 14, 2022

@umohnani8 I don’t know. I keep rebasing this and… crickets. At this point I’m basically resigned to the bot closing it.

@umohnani8
Copy link

Changes LGTM
@sttts @adambkaplan PTAL

@umohnani8
Copy link

@deads2k @mfojtik @sttts PTAL

adambkaplan
adambkaplan approved these changes Apr 22, 2022
View reviewed changes
Copy link
Contributor

@adambkaplan adambkaplan 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 Apr 22, 2022
@mfojtik
Copy link
Contributor

mfojtik commented Apr 28, 2022

/approve

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Apr 28, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: adambkaplan, mfojtik, mtrmac

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 Apr 28, 2022
@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

2 similar comments
@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Apr 28, 2022

@mtrmac: all tests passed!

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

@openshift-merge-robot openshift-merge-robot merged commit 2d6dd57 into openshift:master Apr 28, 2022
@mtrmac mtrmac deleted the use-runtime-utils branch April 29, 2022 14:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adambkaplan adambkaplan adambkaplan approved these changes

@sttts sttts Awaiting requested review from sttts

Assignees

@adambkaplan adambkaplan

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.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Use openshift/runtime-utils/pkg/registries and containers/image instead local copies
7 participants
@mtrmac @openshift-ci-robot @umohnani8 @openshift-bot @mfojtik @adambkaplan @openshift-merge-robot