Skip to content

Conversation

@colebow
Copy link
Member

@colebow colebow commented Feb 22, 2023

Description

We've fully deprecated the memsql name, so we should change the name of the .rst file so it appears as "singlestore.html" in the browser URL. Tested & built locally, doesn't seem as if anything else links to this page.

Additional context and related issues

Follow-up to #16180

Release notes

(x) This is not user-visible or docs only and no release notes are required.
( ) Release notes are required, please propose a release note for me.
( ) Release notes are required, with the following suggested text:

@cla-bot cla-bot bot added the cla-signed label Feb 22, 2023
@colebow colebow requested a review from ebyhr February 22, 2023 23:11
Copy link
Member

@ebyhr ebyhr left a comment

Choose a reason for hiding this comment

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

doesn't seem as if anything else links to this page.

I didn't rename this page intentionally because I have concerns about external usages (=not managed in this repository). We don't care about it? Relates to #15467

@mosabua
Copy link
Member

mosabua commented Feb 23, 2023

Yeah .. we want to look at the redirect stuff in parallel

@colebow
Copy link
Member Author

colebow commented Feb 23, 2023

doesn't seem as if anything else links to this page.

I didn't rename this page intentionally because I have concerns about external usages (=not managed in this repository). We don't care about it? Relates to #15467

It's possible some pages aren't indexed by google, but I can't find an external link to our memsql/singlestore docs at all.

@colebow colebow requested a review from electrum March 1, 2023 22:31
@colebow
Copy link
Member Author

colebow commented Mar 1, 2023

@electrum @jhlodin borrowing from #15467, I've added a redirect plugin to this PR. To allow the build to function for now, I've removed the "-W" flag, which no longer treats build warnings as errors. I've opened up a pull request for the plugin to fix the warning, and once that's included, we can re-add the -W flag.

@jhlodin
Copy link
Contributor

jhlodin commented Mar 2, 2023

Note that this would require a new Sphinx docker image (with the new redirects plugin) to be pushed to ghcr.io, which @electrum handled when we upgraded Sphinx to 5.0.2 last year.

@colebow colebow force-pushed the colebow/refactor-memsql branch 2 times, most recently from 0a86154 to 34692f5 Compare April 10, 2023 19:34
Copy link
Member

Choose a reason for hiding this comment

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

How? Maybe link to issue and also detail what version was modified .. so we can figure it out a bit easier in the future

@colebow colebow force-pushed the colebow/refactor-memsql branch from 34692f5 to f4b7f8d Compare April 11, 2023 19:15
Copy link
Member

@ebyhr ebyhr left a comment

Choose a reason for hiding this comment

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

The commit order is wrong. "Add redirects extension ..." should be the 1st.

@colebow colebow force-pushed the colebow/refactor-memsql branch from f4b7f8d to 95740f0 Compare April 12, 2023 17:16
@colebow colebow force-pushed the colebow/refactor-memsql branch from 95740f0 to 473c12a Compare April 28, 2023 23:34
@colebow colebow force-pushed the colebow/refactor-memsql branch from 473c12a to e9f14e6 Compare May 1, 2023 17:12
Copy link
Member

@mosabua mosabua left a comment

Choose a reason for hiding this comment

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

Looks good now. Lets make sure we check upstream now and then and see if they cut a release .. then we can get rid of the inline redirect and build a new container image and use that.

@mosabua mosabua merged commit 6f2156d into trinodb:master May 1, 2023
@github-actions github-actions bot added this to the 416 milestone May 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

5 participants