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 ".github: Create lint-rst.yaml" #16786

Merged
merged 1 commit into from Jul 7, 2021
Merged

Revert ".github: Create lint-rst.yaml" #16786

merged 1 commit into from Jul 7, 2021

Conversation

bmcustodio
Copy link
Contributor

This reverts commit 6eea0ad, which seems to have broken the build, until it is investigated further.

References #16387.

This reverts commit 6eea0ad, which
seems to have broken the build, until it is investigated further.

References #16387.

Signed-off-by: Bruno Miguel Custódio <brunomcustodio@gmail.com>
@bmcustodio bmcustodio requested review from a team as code owners July 5, 2021 15:36
@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Jul 5, 2021
@bmcustodio bmcustodio added area/build Anything to do with the build, more general then area/CI area/documentation Impacts the documentation, including textual changes, sphinx, or other doc generation code. kind/bug This is a bug in the Cilium logic. needs-backport/1.9 labels Jul 5, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from master in 1.10.3 Jul 5, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from master in 1.9.9 Jul 5, 2021
@qmonnet qmonnet added the release-note/ci This PR makes changes to the CI. label Jul 5, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Jul 5, 2021
Copy link
Member

@qmonnet qmonnet left a comment

Choose a reason for hiding this comment

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

Cc @geyslan for information.

This is not the build, but the GitHub action to invoke the RST linter that currently cause the CI checks to fail. The author of the initial PR seems to have a fix, but at this time it is not clear what that action brings over the rstcheck run that we already had (it seems that both the GH action for rstcheck and rstcheck run in shell scripts were proposed at the same time, and were both accepted (with some delay) and nobody noticed).

Marked for backports because there are pending backports for the initial commits, but it's maybe still time to take the initial commit out of those backports - in which case the current PR won't need to be backported, either.

@geyslan
Copy link
Contributor

geyslan commented Jul 5, 2021

Cc @geyslan for information.

This is not the build, but the GitHub action to invoke the RST linter that currently cause the CI checks to fail. The author of the initial PR seems to have a fix, but at this time it is not clear what that action brings over the rstcheck run that we already had (it seems that both the GH action for rstcheck and rstcheck run in shell scripts were proposed at the same time, and were both accepted (with some delay) and nobody noticed).

Marked for backports because there are pending backports for the initial commits, but it's maybe still time to take the initial commit out of those backports - in which case the current PR won't need to be backported, either.

@qmonnet I got it. I was assigned to the #13366 and followed the instructions put there. =)

Well, could you guys please analyze it and let us know if the #16387 will be reverted (as it's already flagged)? Whatever the decision, thanks a million.

@maintainer-s-little-helper maintainer-s-little-helper bot removed this from Needs backport from master in 1.10.3 Jul 6, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot removed this from Needs backport from master in 1.9.9 Jul 6, 2021
@joestringer
Copy link
Member

Let's merge this revert so that folks don't get unrelated failures on their PRs.

@geyslan once we've merged this, could you try pulling back in the original commit, fixing up this issue and posting a fresh PR? It looks like you already have some idea in mind about how to fix this.

@joestringer joestringer merged commit fe6ba90 into cilium:master Jul 7, 2021
@qmonnet
Copy link
Member

qmonnet commented Jul 8, 2021

Again, before we get a fresh PR it would be good to see if the new action would bring more checks when compared with the rstcheck run that is already in the tree. No need to work on a fix if it's for a redundant check.

@geyslan
Copy link
Contributor

geyslan commented Jul 8, 2021

@qmonnet I agree. Perhaps your solution has already solved the original issue.

@qmonnet
Copy link
Member

qmonnet commented Jul 8, 2021

Looking into this:

  • The checks seem to be the same overall, with a few minor differences:

    • They're not integrated the same, yours was a separate GitHub action while I just updated the Documentation/check-build.sh script instead to integrate it into the existing action for documentation checks.
    • I report on the info level (you do error), but specifically exclude a number of messages that we consider as false positives.
    • I pin a version of rstcheck, while you pull the one available from the Ubuntu repo I think.
  • The issue you initially wanted to address, Catch malformed releases table before merging #13366, detecting malformed tables like this one is not currently addressed. This is because:

    1. The script does not check the main README.rst, which is not under Documentation/ (but it is easy enough to make the script check it too).
    2. Currently, we explicitly omit the reports on malformed tables. This is because this raises false positives when tables contain substitution references. This is a bug in rstcheck, and there is an open issue on the topic. Once this issue if fixed and a new version is released, we should be able to stop ignoring this warning. Alas I don't have anything better for now. :/

In that context, I don't believe a new action is necessary. But you're welcome to address i. and add the main README.rst to the files we check, if you want.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/build Anything to do with the build, more general then area/CI area/documentation Impacts the documentation, including textual changes, sphinx, or other doc generation code. kind/bug This is a bug in the Cilium logic. release-note/ci This PR makes changes to the CI.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants