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

[v1.13] docs: Ignore Helm values, update spelling list #26906

Merged
merged 5 commits into from Jul 27, 2023
Merged

[v1.13] docs: Ignore Helm values, update spelling list #26906

merged 5 commits into from Jul 27, 2023

Conversation

qmonnet
Copy link
Member

@qmonnet qmonnet commented Jul 18, 2023 •
edited

+ Partial backport for the following PRs (not labelled as backported, since we backport only a minor portion of the changes):

Cc @gandro

Once this PR is merged, you can update the PR labels via:

for pr in 26759 24099 23752 24014 26892; do contrib/backporting/set-labels.py $pr done 1.13; done

or with

make add-labels BRANCH=v1.13 ISSUES=26759,24099,23752,24014,26892

@qmonnet qmonnet added kind/backports This PR provides functionality previously merged into master. backport/1.13 This PR represents a backport for Cilium 1.13.x of a PR that was merged to main. labels Jul 18, 2023
@qmonnet qmonnet requested a review from a team as a code owner July 18, 2023 22:53
@qmonnet qmonnet changed the title Pr/qmonnet/1.13 backport 26759 [v1.13] docs: Ignore Helm values, update spelling list Jul 18, 2023
@qmonnet
Copy link
Member Author

qmonnet commented Jul 18, 2023 •
edited

Hmmm. I'm also getting the errors Sebastian reported:

ERROR: Unknown interpreted text role "spelling:ignore".

These are not because we omitted to ignore the spelling:ignore role for rstcheck, as I previously believed. This is because we use sphinxcontrib-spelling v7.6.0, as per Documentation/requirements.txt (and not v7.7.0 as specified in Documentation/requirements-min/requirements.txt, because of Dependabot). But the role we're trying to use was only added in v7.7.0, so in our case the CI legitimately complains that it doesn't know it.

I see two ways forward:

  • Drop backports for <= 1.13 for this PR, the easiest thing to do, even though we'll have to manually re-generate the Helm reference each time we backport something touching Helm values for 1.11, 1.12, 1.13.
  • Backport anyway, meaning we also have to update the version of the Sphinx plug-in. But re-building the docs-builder image locally fails for me on 1.13, so I guess we'd also have to backport a bunch of other commits (0b93dcc, and likely everything related to it); then generate a new docs-builder image for CI for 1.13; then update 1.13 to use that.

I've not much appetite for the second solution - I don't mind spending time on improving the docs framework on main, but collecting all required commits and dependencies to make sure that this works on 1.11--1.13 will likely be painful, so my preference would be to just drop these backports. [Edit] Or maybe it works if we just copy all config/build files for the docs? Such a pain to maintain docs builds on three different branches 😕

@gandro
Copy link
Member

gandro commented Jul 19, 2023 •
edited

Or maybe it works if we just copy all config/build files for the docs?

That would be my preference, but I don't know how hard that is.

Regarding Helm, I've found that it's already often necessary to regenerate the Helm values and README on stable branches, even before this commit. So I'm fine with not backporting it.

But I think it would be helpful to have a guide somewhere that explains to backporters what conflicts they have to expect due to this change, and an short guide on what and how to regenerate.

@qmonnet qmonnet force-pushed the pr/qmonnet/1.13-backport-26759 branch from 08b59b0 to fac93d5 Compare July 24, 2023 11:35
@qmonnet qmonnet requested review from a team as code owners July 24, 2023 11:35
@qmonnet qmonnet force-pushed the pr/qmonnet/1.13-backport-26759 branch from fac93d5 to 4fa83bb Compare July 24, 2023 11:42
@qmonnet
Copy link
Member Author

qmonnet commented Jul 24, 2023

/test-backport-1.13

@qmonnet qmonnet mentioned this pull request Jul 25, 2023
Closed
@qmonnet
Copy link
Member Author

qmonnet commented Jul 25, 2023

/test-1.21-4.19

@qmonnet qmonnet added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jul 25, 2023
@qmonnet qmonnet mentioned this pull request Jul 25, 2023
Merged
11 tasks
@maintainer-s-little-helper maintainer-s-little-helper bot removed the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jul 25, 2023
@qmonnet qmonnet added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jul 25, 2023
@youngnick
Copy link
Contributor

Hey @qmonnet seems like there's now a conflict on helm-values.rst?

@gandro gandro added dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. and removed ready-to-merge This PR has passed all tests and received consensus from code owners to merge. labels Jul 26, 2023
@qmonnet qmonnet force-pushed the pr/qmonnet/1.13-backport-26759 branch from 4fa83bb to a33a42d Compare July 26, 2023 09:01
@qmonnet
Copy link
Member Author

qmonnet commented Jul 26, 2023

There was, I fixed it.

@qmonnet qmonnet removed the dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. label Jul 26, 2023
@nbusseneau nbusseneau added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jul 26, 2023
@nbusseneau
Copy link
Member

This PR only makes changes to the documentation and Helm values, both checks have passed, no need to run full CI for the rebase. Marking ready-to-merge.

@youngnick
Copy link
Contributor

Looks like the release activity is really messing with this one @qmonnet, another conflict in helm-values. Feels like timezones may make this tricky, please feel free to ping me on Slack when you are ready to push the conflict fix and if I'm up, I'll merge asap after that.

@qmonnet
docs: Fix double reference in benchmark docs
e31e82f 
[ upstream commit 0916eac ]

There are two target references (non-anonymous links) names "netperf",
and pointing to the same link anyway. Let's clean it up.

Signed-off-by: Quentin Monnet <quentin@isovalent.com>
@qmonnet
docs: Fix indentation of a table in CI docs
c8753c9 
[ upstream commit daaf1d2 ]

The table should be indented with the item list it belongs to, or Sphinx
will believe the list is finished and open a new one, under the table,
starting from 3.

Signed-off-by: Quentin Monnet <quentin@isovalent.com>
@qmonnet
docs: Backport changes to documentation framework
f609967 
We want to backport some changes touching the structure of the Helm
reference. This requires using a specific role with the spelling plug-in
for Sphinx, which in turn requires upgrading this spelling plug-in to a
newer version. This upgrade requires the creation of a new CI image, but
it seems that building the image locally is broken on 1.13...

Let's just copy most of the Documentation framework files from main to
branch 1.13, and use the same image in CI.

Upstream commits (not sorted):

  - 0b93dcc ("docs: Revert Python to 3.7.9 in docs-builder, downgrade a dependency")
  - 36a31b1 ("Documentation: enable parallel builds")
  - 2e9b20f ("docs: Ignore Helm value names for spellcheck")
  - b445e6e ("docs: Do not print error when spell check output dir is empty")
  - e90df2f ("docs: Update rstcheck")
  - 621dcad ("docs: extract Go version from go.mod")
  - b304ce3 ("Update release scripting for main branch rename")
  - eb1338a ("docs: Update build dependencies (Sphinx add-ons etc.)")
  - 2390916 ("docs: fixed search for every page")

Signed-off-by: Quentin Monnet <quentin@isovalent.com>
@qmonnet
docs: Ignore Helm value names for spellcheck
9a1f4e6 
[ upstream commit 2e9b20f ]

Updating the list of exceptions for spellchecking each time we add a
Helm value is painfull, and hardly useful at all. It's trivial to mark
these strings as ignored to the spell-checker when building the
reference, so let's just do that.

[ Backport note: We have cherry-picked the changes to the Makefile in a
    previous commit, here we just update the Helm reference to mark the
    values as ignored for the spell checker. ]

Signed-off-by: Quentin Monnet <quentin@isovalent.com>
@qmonnet
docs: Trim down spelling list
df0a686 
[ upstream commit e846b71 ]

We have accumulated a number of words in the spelling list over time,
and some of them are no longer necessary (especially now that we ignore
Helm value names from the Helm reference). Let's trim down the list.

The following process was applied:

    1. Empty the list of spelling exceptions
    2. Run 'make html' to get the command to add missing words
       (update-spelling_wordlist.sh)
    3. Run that command to add all words
    4. Remove all entries starting with an upper case from the list, as
       some of them are unnecessary given that they have a corresponding
       lowercase entry (which is case-insensitive)
    5. Re-run 'make html' to get the command to add the exceptions that
       are still missing
    6. Run that command
    7. Manually edit the file to make sure we don't change/add existing
       entries (no Git addition)
    8. Re-add names of contributors that are recognised locally, but
       that CI fails to recognise because it doesn't have the Git
       history

Signed-off-by: Quentin Monnet <quentin@isovalent.com>
@qmonnet qmonnet force-pushed the pr/qmonnet/1.13-backport-26759 branch from a33a42d to df0a686 Compare July 27, 2023 11:49
@qmonnet
Copy link
Member Author

qmonnet commented Jul 27, 2023

Looks like the release activity is really messing with this one @qmonnet, another conflict in helm-values.

Yeah, there's one commit here that changes all entries in the Helm reference, so as soon as we backport some other change touching that file there's a conflict.

This is also why we want to backport this change, in fact: otherwise, all backports from main to 1.13 touching Helm values will need to regenerate the Helm reference. Anyway... at least it's trivial to rebase, easier than the conflict on the spelling exceptions from yesterday.

please feel free to ping me on Slack when you are ready to push the conflict fix

Sure, will do. We should be good if the Documentation workflow and integration tests pass.

@qmonnet
Copy link
Member Author

qmonnet commented Jul 27, 2023

Looks all in order 🚀

@youngnick youngnick merged commit 0c0d709 into v1.13 Jul 27, 2023
39 checks passed
@youngnick youngnick deleted the pr/qmonnet/1.13-backport-26759 branch July 27, 2023 12:50
@nebril nebril mentioned this pull request Aug 10, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@squeed squeed squeed approved these changes

@geakstr geakstr geakstr approved these changes

@nbusseneau nbusseneau nbusseneau approved these changes

@youngnick youngnick youngnick approved these changes

@viktor-kurchenko viktor-kurchenko viktor-kurchenko approved these changes

Assignees
No one assigned
Labels
backport/1.13 This PR represents a backport for Cilium 1.13.x of a PR that was merged to main. kind/backports This PR provides functionality previously merged into master. ready-to-merge This PR has passed all tests and received consensus from code owners to merge.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@qmonnet @gandro @youngnick @nbusseneau @squeed @geakstr @viktor-kurchenko