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.

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

Revert #3782 "Added Pipelines v2 installation pages" #3791

Merged

Conversation

thesuperzapper
Copy link
Member

@thesuperzapper thesuperzapper commented Jul 5, 2024

Reverts #3782

Please see #3782 (comment).

@thesuperzapper
Copy link
Member Author

@hbelmiro as I was saying in #3782 (comment), the PR #3782 was unacceptable for a few reasons and needs to be reverted ASAP:

  1. It made no actual content changes while moving pages from the V1 to the V2 section
  2. It added pages to the V2 section which have no relationship to the Kubeflow Pipelines V2 (like "Choose an executor").
  3. It removed the docs for installing KFP v1.
  4. It did not add any redirects making all old links into 404s.

@thesuperzapper thesuperzapper force-pushed the revert-3782-kfpv2-install branch 2 times, most recently from 2bd812b to 5da10e2 Compare July 5, 2024 23:48
@thesuperzapper
Copy link
Member Author

@andreyvelich @rimolive either of you could also approve this PR, because it's very important that we revert soon.

@thesuperzapper
Copy link
Member Author

Also @kubeflow/pipelines might be interested to see that we are reverting a strange PR in the KFP docs.

@rimolive
Copy link
Member

rimolive commented Jul 5, 2024

@thesuperzapper Why reverting the change instead of fixing the missing links? I just checked the docs and I could not find any missing/broken link so please point me to these ones so we can fix asap.

@thesuperzapper
Copy link
Member Author

thesuperzapper commented Jul 6, 2024

You are not reading my comment #3791 (comment), the primary issue is that it's not acceptable to move V1 docs into the V2 section without regard for the content.

Also, here is a list of links that are now 404:

But even if you had created the appropriate redirects, I would still be suggesting a revert, because it's just not helpful to move docs so that they become incorrect (in the wrong section).

@rimolive
Copy link
Member

rimolive commented Jul 6, 2024

KFP docs are still WIP and there's a lot of work tracked in this issue. #3712

Reverting this change will block us on #3729 and this is critical as this is part of the Kubeflow 1.9 change.

@thesuperzapper
Copy link
Member Author

@rimolive please don't try and defend the PR, just accept the reverse and make a new one, everyone makes mistakes.

It was never acceptable to randomly move V1 docs into the V2 section.

@hbelmiro
Copy link
Contributor

hbelmiro commented Jul 6, 2024

@rimolive @thesuperzapper I'm approving this revert and will create a new installation page for v2 with minimal information to install v2. We can iterate later.

/lgtm
/approve

Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hbelmiro

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

@hbelmiro
Copy link
Contributor

hbelmiro commented Jul 6, 2024

@thesuperzapper can you please rebase?

This reverts commit 5765bfa

Signed-off-by: Mathew Wicks <5735406+thesuperzapper@users.noreply.github.com>
@google-oss-prow google-oss-prow bot removed the lgtm label Jul 6, 2024
@thesuperzapper thesuperzapper changed the title Revert "Added Pipelines v2 installation pages" Revert #3782 "Added Pipelines v2 installation pages" Jul 6, 2024
@thesuperzapper
Copy link
Member Author

@hbelmiro I have rebased the PR.

Copy link
Contributor

@hbelmiro hbelmiro left a comment

Choose a reason for hiding this comment

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

/lgtm

@google-oss-prow google-oss-prow bot added the lgtm label Jul 6, 2024
@google-oss-prow google-oss-prow bot merged commit 3723136 into kubeflow:master Jul 6, 2024
6 checks passed
@thesuperzapper thesuperzapper deleted the revert-3782-kfpv2-install branch July 6, 2024 19:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants