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

Documentation: dont use docker for check-cmdref #16939

Merged
merged 1 commit into from Aug 2, 2021

Conversation

kkourt
Copy link
Contributor

@kkourt kkourt commented Jul 20, 2021

check-cmdref just uses bash and git, so there is no reason to run it in
a container.

At the same time, running it in a container causes the script to not
work when using it in a directory created with git clone --reference,
because in this case git object live outside of the directory that is
mounted in the docker container and git diff will fail.

Signed-off-by: Kornilios Kourtis kornilios@isovalent.com

check-cmdref just uses bash and git, so there is no reason to run it in
a container.

At the same time, running it in a container causes the script to not
work when using it in a directory created with git clone --reference,
because in this case git object live outside of the directory that is
mounted in the docker container and git diff will fail.

Signed-off-by: Kornilios Kourtis <kornilios@isovalent.com>
@kkourt kkourt requested a review from a team as a code owner July 20, 2021 10:22
@kkourt kkourt requested a review from joestringer July 20, 2021 10:22
@kkourt kkourt added the release-note/misc This PR makes changes that have no direct user impact. label Jul 20, 2021
Copy link
Member

@joestringer joestringer left a comment

Choose a reason for hiding this comment

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

LGTM. Will request @errordeveloper to also take a look since he introduced this change originally.

Copy link
Contributor

@errordeveloper errordeveloper left a comment

Choose a reason for hiding this comment

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

So from my point of view, the documentation ought to be containerised to isolate Python from the rest of the build dependencies, which have nothing to do with Python. That idea was extended to "anything to do with Documentation/ belongs to documentation build and should thereby be done in the same container", which matters in practice when it comes to cross-builds, e.g. a macOS or Windows user may wish to run documentation checks against cross-compiled Linux binaries (because Cilium only compiles for Linux), and they'd only be able to do that if the checks run in a Linux container.
From what it looks like, the main part of documentation build remains unchanged, so Python is remains an isolated dependency, and it's only some elements of the documentation build will be executed outside of a container. Either way, I don't object on the basis of my theoretical/idealist view, if folks have good reasons to make any changes to the documentation build, I won't object.

@kkourt
Copy link
Contributor Author

kkourt commented Jul 28, 2021

Thanks @errordeveloper. Indeed there is a concrete use-case for this, which is having mulitple git repositories that share their objects. As you said, the documentation is still build within a container, it is only the check on whether we need to update things that has changed.

I'll mark this ready-to-merge

@kkourt kkourt added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jul 28, 2021
@kkourt
Copy link
Contributor Author

kkourt commented Jul 28, 2021

Note that the changes only changes a check to be executed outside of a container, so no further testing is required.

@vadorovsky vadorovsky merged commit 34e357e into cilium:master Aug 2, 2021
@kkourt kkourt deleted the pr/doc-cmdref branch August 2, 2021 14:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/misc This PR makes changes that have no direct user impact.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants