Skip to content

BZ-1910319: updated paths#28919

Closed
skrthomas wants to merge 1 commit intoopenshift:masterfrom
skrthomas:BZ1910319
Closed

BZ-1910319: updated paths#28919
skrthomas wants to merge 1 commit intoopenshift:masterfrom
skrthomas:BZ1910319

Conversation

@skrthomas
Copy link
Copy Markdown
Contributor

@skrthomas skrthomas commented Jan 28, 2021

@openshift-ci-robot openshift-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Jan 28, 2021
@netlify
Copy link
Copy Markdown

netlify Bot commented Jan 28, 2021

Deploy preview for osdocs ready!

Built with commit 224e3c3

https://deploy-preview-28919--osdocs.netlify.app

Copy link
Copy Markdown
Contributor

@bergerhoffer bergerhoffer left a comment

Choose a reason for hiding this comment

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

Just two questions that we need to run by SME/QE!

ifndef::openshift-origin[]
-a ${REG_CREDS} \
endif::[]
--path /usr/bin/registry/darwin-amd64-opm:. \
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@skrthomas So it looks like you removed the registry/ path which is good, but you also changed the name of the binary from darwin-amd64-opm to just opm.

I'm not sure if that change is appropriate or not, but that's something that I would ask @zhouying7780 to confirm.

Or maybe @adellape knows a good SME on the Operator team to ask?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Just double-checked, and they should remain as darwin-amd64-opm and windows-amd64-opm for those binaries.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

$ oc image extract registry.redhat.io/openshift4/ose-operator-registry:v4.6     -a ${REG_CREDS}     --path /usr/bin/windows-amd64-opm:. --confirm

W0128 10:31:30.601994  176229 manifest.go:414] Chose linux/amd64 manifest from the list.
To include the manifest list digest: sha256:490035a0394c1f6aa32f193b61a28f983801ef467339729d4a724bb4b3ec7f7e, use --keep-manifest-list=true

$ oc image extract registry.redhat.io/openshift4/ose-operator-registry:v4.6     -a ${REG_CREDS}     --path /usr/bin/darwin-amd64-opm:. --confirm

W0128 10:32:10.349443  176319 manifest.go:414] Chose linux/amd64 manifest from the list.
To include the manifest list digest: sha256:490035a0394c1f6aa32f193b61a28f983801ef467339729d4a724bb4b3ec7f7e, use --keep-manifest-list=true

$ ll
total 156072
-rw-r-----. 1 durant durant 67038328 Jan 15 19:30 darwin-amd64-opm
-rw-r-----. 1 durant durant 92769624 Jan 15 19:31 windows-amd64-opm

-a ${REG_CREDS} \
endif::[]
--path /usr/bin/registry/windows-amd64-opm:. \
--path /usr/bin/opm:. \
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Same question for @zhouying7780 if the binary needs to stay windows-amd64-opm or if it should just be opm for windows.

@bergerhoffer bergerhoffer added the peer-review-done Signifies that the peer review team has reviewed this PR label Jan 28, 2021
@bergerhoffer
Copy link
Copy Markdown
Contributor

@skrthomas Also, do you know which OCP versions this is applicable for?

@skrthomas
Copy link
Copy Markdown
Contributor Author

@bergerhoffer 4.6/4.7

@skrthomas skrthomas closed this Jan 28, 2021
@vikram-redhat
Copy link
Copy Markdown
Contributor

Hey @skrthomas - leave a comment here as to why you closed this PR and provide the link to the followup PRs if any. When we close PRs, also remove the milestones and labels (request the peer reviewers or anyone in the team to do so).

@skrthomas
Copy link
Copy Markdown
Contributor Author

@vikram-redhat will do! I closed this one because, in discussion with @adellape, we realized that this fix acutally needed to be in enterprise_4.5 and enterprise_4.6. Here are the links to the 2 PRs creates in place of closing this one:
#28933
#28935

@bergerhoffer
Copy link
Copy Markdown
Contributor

@vikram-redhat I don't think that I was aware that we remove labels when we close PRs, is there a reason why we'd need to do this? If it's for querying reasons, I'd think that you should be able to filter out closed ones pretty easily.

But if there is a good reason, then we should communicate this around the team, because I missed the memo!

@vikram-redhat
Copy link
Copy Markdown
Contributor

@bergerhoffer it is indeed to do with querying and reporting, and it just makes it easier at the end of the release to identify everything that we did end up doing.

I have mentioned this in a few of the weekly meetings (for example, 13th July), but I have neglected to add to the manual. Will do so today.

@bergerhoffer
Copy link
Copy Markdown
Contributor

I have mentioned this in a few of the weekly meetings (for example, 13th July), but I have neglected to add to the manual. Will do so today.

I'm going to pretend I was on PTO that day 😉

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

Labels

peer-review-done Signifies that the peer review team has reviewed this PR size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants