Skip to content

Ocp migration new #17516

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
Merged
bergerhoffer merged 1 commit into from
Oct 30, 2019
Merged

Ocp migration new #17516

bergerhoffer merged 1 commit into from
Oct 30, 2019

Conversation

@apinnick
Copy link
Contributor

@apinnick apinnick commented Oct 22, 2019 •
edited by ahardin-rh
Loading

@openshift-ci-robot openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Oct 22, 2019
@apinnick apinnick mentioned this pull request Oct 22, 2019
Closed
@gildub
Copy link

gildub commented Oct 22, 2019

@apinnick,

First off, kudos for this hard work!

In the "Using CPMA" - "Procedure" section we actually need to add between 1. and 2. (either as 1 subsection or pushing 2. down) a paragraph about ssh permission and also grant privileges.

Please see https://github.com/fusor/cpma/#authentication-and-authorization for more details and/or don't hesitate to ask me.

gildub
gildub reviewed Oct 22, 2019
View reviewed changes
Copy link

@gildub gildub left a comment

Choose a reason for hiding this comment

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

That's exactly what we need but maybe it belongs to Procedure section.

bergerhoffer
bergerhoffer reviewed Oct 22, 2019
View reviewed changes
Copy link
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.

@apinnick I'm not sure whether you were ready for a peer review, but I heard that this is going to go out this week, and it needed to be peer reviewed and approved after addressing feedback before it could get merged. So I wanted to make sure we had enough time.

Let me know if you have any questions on the feedback. Thanks!

One other item that I couldn't comment directly inline about is that there are darkcircle*.png files, that don't seem to be used. Can you remove these?

@bergerhoffer
Copy link
Contributor

bergerhoffer commented Oct 22, 2019

@apinnick Can you also specify what version(s) this is for? Is it for 4.2+, or is it for 4.1 as well?

@apinnick
Copy link
Contributor Author

apinnick commented Oct 22, 2019

@bergerhoffer It's for 4.2+

@apinnick
Copy link
Contributor Author

apinnick commented Oct 22, 2019 •
edited
Loading

@bergerhoffer

@apinnick I'm not sure whether you were ready for a peer review, but I heard that this is going to go out this week, and it needed to be peer reviewed and approved after addressing feedback before it could get merged. So I wanted to make sure we had enough time.

Not ready for peer review. Still need some input on Noobaa.

Let me know if you have any questions on the feedback. Thanks!

One other item that I couldn't comment directly inline about is that there are darkcircle*.png files, that don't seem to be used. Can you remove these?

They're used in the CR section. I just added them.

jwmatthews
jwmatthews reviewed Oct 22, 2019
View reviewed changes
@openshift-ci-robot openshift-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Oct 23, 2019
@apinnick
Copy link
Contributor Author

apinnick commented Oct 23, 2019

Yes, just found out that link was old. I've updated the file names

@bergerhoffer bergerhoffer added this to the Next Release milestone Oct 23, 2019
ahardin-rh
ahardin-rh reviewed Oct 23, 2019
View reviewed changes
ahardin-rh
ahardin-rh reviewed Oct 23, 2019
View reviewed changes
ahardin-rh
ahardin-rh reviewed Oct 23, 2019
View reviewed changes
ahardin-rh
ahardin-rh reviewed Oct 23, 2019
View reviewed changes
@apinnick
Copy link
Contributor Author

apinnick commented Oct 24, 2019

@bergerhoffer If I can't use xrefs in modules, can I use footnotes?

One of the devs wants me to add a line (basically a short procedure) for granting cluster-admin privileges to a user. I told him that a procedure, even a single line, would look weird in a list of prerequisites and he asked whether a link could be provided instead. Since we can't have xrefs in modules, should I put the line in a footnote?

@apinnick
Copy link
Contributor Author

apinnick commented Oct 24, 2019

@bergerhoffer I can't seem to comment on your comment on ./cpma, so here's the comment:

Is "cpma" a directory or a command? The way it's used above, it's an executable file.
And the "." in "./cpma" means "in the current directory", so it's a relative path.

I did some digging and got more info on exactly what the user is downloading. I also updated the installation procedure because it was missing some details. It's different from our normal installations (like yum or rpm).

cpma is the command and is the name of a binary file that the user downloads and saves in a shared binary directory ($PATH for Linux/MacOSX, %PATH% for Windows). That means it can be run from any directory.

./ in this context is not a path. It indicates that the binary is supposed to run in the current directory, which is not defined as $PATH or %PATH%. See https://unix.stackexchange.com/questions/4430/why-do-we-use-to-execute-a-file for more info.

@apinnick
Copy link
Contributor Author

apinnick commented Oct 24, 2019

@bergerhoffer @ahardin-rh

I just heard from Marco, the PM, that OCP migration GA has been postponed until middle of next week.

@openshift-ci-robot openshift-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Oct 24, 2019
apinnick
apinnick commented Oct 29, 2019
View reviewed changes
apinnick
apinnick commented Oct 29, 2019
View reviewed changes
mberube99
mberube99 reviewed Oct 29, 2019
View reviewed changes
@xinredhat
Copy link

xinredhat commented Oct 29, 2019

@apinnick
Would you please clarify that the update happens while creating Migration Controller?

Update the following values in the Migration controller CR:

For the error/critical message of migration plan limits, they will appear in the page. This feature already is implemented in PR migtools/mig-ui#604

@apinnick
Copy link
Contributor Author

apinnick commented Oct 29, 2019

@apinnick
Would you please clarify that the update happens while creating Migration Controller?

Update the following values in the Migration controller CR:

Why do they have to change the parameters while creating the Migration controller? Why can't they edit the CR?

For the error/critical message of migration plan limits, they will appear in the page. This feature already is implemented in PR fusor/mig-ui#604

Yes, I just received the screenshots. I'm going to use the UI messages instead of the CR content because we can assume that the user is more likely to see the warnings in the UI than in the CR.

@bergerhoffer
Copy link
Contributor

bergerhoffer commented Oct 29, 2019

@apinnick Is Thursday still the GA date for this? And are you finished with additional updates? I can try to run through it again now, but we need to leave time to get a final peer review, and address feedback from that, after any other changes are made.

In the future, it would be good to submit the initial content, and get it peer reviewed/merged, and then follow up in separate PRs for additional updates. Having one large PR makes it more difficult to peer review and keep up with the changes. And runs into the risk of it not being ready to go out the day it needs to, since we need to do a final peer review before merging.

What other teams have been doing when adding content that isn't ready to be published is adding the content, but commenting out the new files in the _topic_map.yml file. So I'd recommend that for next time.

@bergerhoffer
Copy link
Contributor

bergerhoffer commented Oct 29, 2019

As of the changes right now, this looks good to me.

eriknelson
eriknelson suggested changes Oct 29, 2019
View reviewed changes
@apinnick apinnick force-pushed the ocp-migration-new branch 2 times, most recently from d919753 to 355ac60 Compare October 30, 2019 07:02
@apinnick apinnick marked this pull request as ready for review October 30, 2019 07:05
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 30, 2019
@apinnick
Copy link
Contributor Author

apinnick commented Oct 30, 2019

@bergerhoffer This PR is ready for final review and merge. Thanks.

@openshift-ci-robot openshift-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Oct 30, 2019
@apinnick
OCP 3 to 4 migration
7269411 
This reverts commit 355ac60.

This reverts commit cade142.
@bergerhoffer
Copy link
Contributor

bergerhoffer commented Oct 30, 2019

LGTM, merging!

@bergerhoffer bergerhoffer added the peer-review-done Signifies that the peer review team has reviewed this PR label Oct 30, 2019
@bergerhoffer bergerhoffer merged commit a29b497 into openshift:master Oct 30, 2019
@bergerhoffer
Copy link
Contributor

bergerhoffer commented Oct 30, 2019

/cherrypick enterprise-4.2

@bergerhoffer
Copy link
Contributor

bergerhoffer commented Oct 30, 2019

/cherrypick enterprise-4.3

@openshift-cherrypick-robot openshift-cherrypick-robot mentioned this pull request Oct 30, 2019
Merged
@openshift-cherrypick-robot
Copy link

openshift-cherrypick-robot commented Oct 30, 2019

@bergerhoffer: new pull request created: #17723

In response to this:

/cherrypick enterprise-4.2

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 openshift-cherrypick-robot mentioned this pull request Oct 30, 2019
Merged
@openshift-cherrypick-robot
Copy link

openshift-cherrypick-robot commented Oct 30, 2019

@bergerhoffer: new pull request created: #17724

In response to this:

/cherrypick enterprise-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.

@apinnick apinnick deleted the ocp-migration-new branch December 16, 2019 09:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bergerhoffer bergerhoffer bergerhoffer left review comments

@ahardin-rh ahardin-rh ahardin-rh left review comments

+7 more reviewers

@jwmatthews jwmatthews jwmatthews left review comments

@sferich888 sferich888 sferich888 left review comments

@gildub gildub gildub left review comments

@vikram-redhat vikram-redhat vikram-redhat left review comments

@mberube99 mberube99 mberube99 left review comments

@xinredhat xinredhat xinredhat left review comments

@eriknelson eriknelson eriknelson requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

branch/enterprise-4.2 branch/enterprise-4.3 peer-review-done Signifies that the peer review team has reviewed this PR size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.

Projects

None yet

Milestone

Next Release

Development

Successfully merging this pull request may close these issues.