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

Bug 1772942: Fix ignored --manifest-dir option in oc adm catalog build #181

Merged
merged 1 commit into from Dec 18, 2019
Merged

Bug 1772942: Fix ignored --manifest-dir option in oc adm catalog build #181

merged 1 commit into from Dec 18, 2019

Conversation

njhale
Copy link
Contributor

@njhale njhale commented Nov 25, 2019 •
edited

Bumps the operator-registry dependency to a version that handles the cache directory option correctly.

Also fixes a bad pseudoversion for the module replacement of openshift/api which was causing module operations to fail. Looks like this was fixed in another PR.

@openshift-ci-robot
Copy link

@njhale: This pull request references Bugzilla bug 1772942, which is invalid:

  • expected dependent Bugzilla bug 1775527 to be in one of the following states: VERIFIED, RELEASE_PENDING, CLOSED (ERRATA), but it is ON_QA instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

Bug 1772942: Fix ignored --manifest-dir option in oc adm catalog build

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.

@openshift-ci-robot openshift-ci-robot added bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Nov 25, 2019
@njhale
Copy link
Contributor Author

njhale commented Dec 4, 2019

/bugzilla refresh
/cherry-pick release-4.3

@openshift-cherrypick-robot
Copy link

@njhale: once the present PR merges, I will cherry-pick it on top of release-4.3 in a new PR and assign it to you.

In response to this:

/bugzilla refresh
/cherry-pick release-4.3

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.

@openshift-ci-robot openshift-ci-robot added bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. and removed bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. labels Dec 4, 2019
@openshift-ci-robot
Copy link

@njhale: This pull request references Bugzilla bug 1772942, which is valid. The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

/bugzilla refresh
/cherry-pick release-4.3

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.

@njhale
Copy link
Contributor Author

njhale commented Dec 4, 2019

/assign @tnozicka

soltysh
soltysh requested changes Dec 4, 2019
View reviewed changes
Copy link
Member

@soltysh soltysh left a comment

Choose a reason for hiding this comment

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

/hold
Can you outline the exact steps you invoked I'm seeing more changes than I'd anticipate?

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 4, 2019
@njhale
Copy link
Contributor Author

njhale commented Dec 4, 2019 •
edited

@soltysh IIRC I bumped the operator-registry dependency by modifying the minor version in the go.mod followed by go mod tidy and go vendor. However, before I could run these commands, I needed to alter the replace directive for openshift/api, since the given pseudoversion (v0.0.0-0.20191112184635-86def77f6f) was causing the commands to fail. Hence the PR description:

Also fixes a bad pseudoversion for the module replacement of openshift/api which was causing module operations to fail.

Other than that, are there additional changes you see that you weren't expecting?

I noticed a goodmod.yaml file, but didn't see any make rules using the related tool for vendoring. If there is a preferred process for updating oc deps, I'd be happy to follow it -- just point me in the right direction (may have missed the docs) 🙂

@njhale
Copy link
Contributor Author

njhale commented Dec 6, 2019

@soltysh Just following up on my previous post; any thoughts, questions?

@soltysh
Copy link
Member

soltysh commented Dec 9, 2019

@njhale so the process you described seems legit (go mod vendor + go mod tidy) but I just did the same on top of your PR and files such as vendor/golang.org/x/sys/unix/mkasm_darwin.go and others I didn't expect to be modified are not in my local copy, whereas somehow they are in yours? Which version of go you used, I've tried both go1.12 and 1.13 and both did not clean those files for me.

@njhale
Copy link
Contributor Author

njhale commented Dec 9, 2019

@soltysh Thanks for getting back to me. I used 1.13: go version go1.13.4 darwin/amd64.

It may have something to do with the OS deps are being pulled onto (mkasm_darwin.go is suspect), or maybe not -- either way, I'll try again and see If I come away without the changes.

@njhale
Copy link
Contributor Author

njhale commented Dec 10, 2019

@soltysh @mfojtik There are several other PRs fixing the pseudo-version issue. If either of those merge first, I'll drop the changes here and try again.

@soltysh
Copy link
Member

soltysh commented Dec 12, 2019

@njhale some changes merged, can you rebase?

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 12, 2019
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 13, 2019
@openshift-ci-robot
Copy link

@njhale: This pull request references Bugzilla bug 1772942, which is valid.

In response to this:

Bug 1772942: Fix ignored --manifest-dir option in oc adm catalog build

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.

@njhale
Copy link
Contributor Author

njhale commented Dec 13, 2019

@soltysh After rebasing, things are looking much better. PTAL.

soltysh
soltysh approved these changes Dec 13, 2019
View reviewed changes
Copy link
Member

@soltysh soltysh left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve
/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 13, 2019
@openshift-bot
Copy link
Contributor

/retest

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

26 similar comments
@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-merge-robot openshift-merge-robot merged commit 574a45f into openshift:master Dec 18, 2019
@openshift-ci-robot
Copy link

@njhale: All pull requests linked via external trackers have merged. Bugzilla bug 1772942 has been moved to the MODIFIED state.

In response to this:

Bug 1772942: Fix ignored --manifest-dir option in oc adm catalog build

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.

@openshift-cherrypick-robot
Copy link

@njhale: #181 failed to apply on top of branch "release-4.3":

.git/rebase-apply/patch:2052: trailing whitespace.
		
.git/rebase-apply/patch:2380: trailing whitespace.
			FOREIGN KEY(replaces) REFERENCES channel_entry(entry_id) DEFERRABLE INITIALLY DEFERRED, 
.git/rebase-apply/patch:2396: space before tab in indent.
     		operatorbundle_name TEXT,
.git/rebase-apply/patch:2397: space before tab in indent.
     		FOREIGN KEY(operatorbundle_name) REFERENCES operatorbundle(name) ON DELETE CASCADE
.git/rebase-apply/patch:2614: space before tab in indent.
     		operatorbundle_name TEXT,
warning: squelched 4 whitespace errors
warning: 9 lines add whitespace errors.
error: Failed to merge in the changes.
Using index info to reconstruct a base tree...
M	go.mod
M	go.sum
M	vendor/modules.txt
Falling back to patching base and 3-way merge...
Auto-merging vendor/modules.txt
CONFLICT (content): Merge conflict in vendor/modules.txt
Auto-merging go.sum
CONFLICT (content): Merge conflict in go.sum
Auto-merging go.mod
CONFLICT (content): Merge conflict in go.mod
Patch failed at 0001 chore(modules): bump operator-registry to 1.5.4

In response to this:

/bugzilla refresh
/cherry-pick release-4.3

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@soltysh soltysh soltysh approved these changes

@mfojtik mfojtik Awaiting requested review from mfojtik

@smarterclayton smarterclayton Awaiting requested review from smarterclayton

Assignees

@soltysh soltysh

@tnozicka tnozicka

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. lgtm Indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@njhale @openshift-ci-robot @openshift-cherrypick-robot @soltysh @openshift-bot @tnozicka @openshift-merge-robot