Run local markdown tests inside an isolated container

[fmuyassarov]

When working on a patch that touches docs/, there is no way to test changes against markdown checks before submitting a PR. In fact, we have a makefile target mdlint but it has a prerequisite of having mdl installed on the developer machine. What do you think of swapping the tasks of mdlint Makefile target with scripts/test-infra/mdlint.sh script with minor modifications so that developer can just run make mdlint which calls scripts/test-infra/mdlint.sh and the tests get executed within a container. Once execution is completed the temporary container gets removed so that we don't have garbage collected. I've chosen ruby:slim image (70.39 MB), which is almost five times smaller than ruby:2.7 (321.36 MB) that we use in our Prow tests.

[k8s-ci-robot]

Hi @fmuyassarov. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[ArangoGutierrez]

Test run on an isolated container at https://github.com/kubernetes/test-infra/blob/master/config/jobs/kubernetes-sigs/node-feature-discovery/node-feature-discovery-presubmits.yaml#L38.
If what you want is a devel make target, then you need to have in consideration the above.

[fmuyassarov]

Test run on an isolated container at https://github.com/kubernetes/test-infra/blob/master/config/jobs/kubernetes-sigs/node-feature-discovery/node-feature-discovery-presubmits.yaml#L38.

If what you want is a devel make target, then you need to have in consideration the above.

Yes, so my plan is run it in a container when testing locally. If I am not wrong, this won't break Prow tests too, it is just that there will be an extra container created within prowJob container.

[marquiz]

I think the idea makes sense. Makes life easier for most of the people

If I am not wrong, this won't break Prow tests too, it is just that there will be an extra container created within prowJob container.

I think the idea behind he scripts under scripts/test-infra has been to keep them as simple and possible and only have strictly prow-specific stuff there. One possibility:

1. put the one-liner from the makefile into a separate script (say scripts/mdlint.sh)
2. call this^ script from scripts/test-infra/mdlint.sh
3. write another script for running mdlint from a container (say hack/mdlint-containerized.sh that runs scripts/mdlint.sh inside a container)
4. Makefile mdlint: target would call hack/mdlint-containerized.sh

WDYT?

[fmuyassarov]

I think the idea makes sense. Makes life easier for most of the people

If I am not wrong, this won't break Prow tests too, it is just that there will be an extra container created within prowJob container.

I think the idea behind he scripts under scripts/test-infra has been to keep them as simple and possible and only have strictly prow-specific stuff there. One possibility:

Got it.

1. put the one-liner from the makefile into a separate script (say scripts/mdlint.sh)
2. call this^ script from scripts/test-infra/mdlint.sh
3. write another script for running mdlint from a container (say hack/mdlint-containerized.sh that runs scripts/mdlint.sh inside a container)
4. Makefile mdlint: target would call hack/mdlint-containerized.sh

WDYT?

Yes this should work and I'm fine with this. I will just mention that, we can reuse the same scripts/test-infra/mdlint.sh for both local and Prow tests. When running on Prow, we just have to add variable in test infra with IS_CONTAINER=true. What I don't like with duplication is that we always have to take care two sets of the same things and we have to be careful that they don't diverge. But as said, it is not a big deal at all and I'm ok to decouple the piece for local runs.

[marquiz]

Got it.

And to put it in other words, the stuff under scripts/test-infra is only supposed to be called from prow 😄

1. put the one-liner from the makefile into a separate script (say scripts/mdlint.sh)
2. call this^ script from scripts/test-infra/mdlint.sh
3. write another script for running mdlint from a container (say hack/mdlint-containerized.sh that runs scripts/mdlint.sh inside a container)
4. Makefile mdlint: target would call hack/mdlint-containerized.sh

WDYT?

Yes this should work and I'm fine with this. I will just mention that, we can reuse the same scripts/test-infra/mdlint.sh for both local and Prow tests. When running on Prow, we just have to add variable in test infra with IS_CONTAINER=true. What I don't like with duplication is that we always have to take care two sets of the same things and we have to be careful that they don't diverge. But as said, it is not a big deal at all and I'm ok to decouple the piece for local runs.

Yeah, I think this is fine. No need to have two separate scripts

[marquiz]

Got it.

And to put it in other words, the stuff under scripts/test-infra is only supposed to be called from prow 😄

1. put the one-liner from the makefile into a separate script (say scripts/mdlint.sh)
2. call this^ script from scripts/test-infra/mdlint.sh
3. write another script for running mdlint from a container (say hack/mdlint-containerized.sh that runs scripts/mdlint.sh inside a container)
4. Makefile mdlint: target would call hack/mdlint-containerized.sh

WDYT?

Yes this should work and I'm fine with this. I will just mention that, we can reuse the same scripts/test-infra/mdlint.sh for both local and Prow tests. When running on Prow, we just have to add variable in test infra with IS_CONTAINER=true. What I don't like with duplication is that we always have to take care two sets of the same things and we have to be careful that they don't diverge. But as said, it is not a big deal at all and I'm ok to decouple the piece for local runs.

Yeah, I think this is fine. No need to have two separate scripts

Hmm, actually what I don't like in this is the recursive nature of the script. With a small mistake end up with infinite loop of nested containers... There should be no duplication with the scripts: one script simply runs mdlint, another script runs the first one in a container environment, and test-infra script simply calls the first one as well. WDYT?

[fmuyassarov]

Got it.

And to put it in other words, the stuff under scripts/test-infra is only supposed to be called from prow smile

1. put the one-liner from the makefile into a separate script (say scripts/mdlint.sh)
2. call this^ script from scripts/test-infra/mdlint.sh
3. write another script for running mdlint from a container (say hack/mdlint-containerized.sh that runs scripts/mdlint.sh inside a container)
4. Makefile mdlint: target would call hack/mdlint-containerized.sh

WDYT?

Yes this should work and I'm fine with this. I will just mention that, we can reuse the same scripts/test-infra/mdlint.sh for both local and Prow tests. When running on Prow, we just have to add variable in test infra with IS_CONTAINER=true. What I don't like with duplication is that we always have to take care two sets of the same things and we have to be careful that they don't diverge. But as said, it is not a big deal at all and I'm ok to decouple the piece for local runs.

Yeah, I think this is fine. No need to have two separate scripts

Hmm, actually what I don't like in this is the recursive nature of the script. With a small mistake end up with infinite loop of nested containers... There should be no duplication with the scripts: one script simply runs mdlint, another script runs the first one in a container environment, and test-infra script simply calls the first one as well. WDYT?

Sorry perhaps I confused you. But I'm not changing how we run tests in Prow, because in prow config, we will have IS_CONTAINER=true which will execute

if [ "${IS_CONTAINER}" != "false" ]; then
    # Install mdl
    gem install mdl -v 0.11.0
    # Run verify steps
    find docs/ -path docs/vendor -prune -false -o -name '*.md' -exec mdl -s docs/mdl-style.rb {} \+ 

Basically nothing changes from the Prow's perspective and we are not having nested containers.

As a developer who wants to test locally, we would just run make mdlint which will create a container and
run the tests inside, exactly the same as Prow (1:1). So, this patch is only an addition I would say.

[fmuyassarov]

As a developer who wants to test locally, we would just run make mdlint which will create a container and run the tests inside, exactly the same as Prow (1:1). So, this patch is only an addition I would say.

It is the same as Prow, because ProwJob also creates the same temporary container with the same container image that we are using here(ruby:slim)

[fmuyassarov]

I've updated kubernetes/test-infra#27376 to match the changes. Let know what you think. Thanks

[fmuyassarov]

Thanks for reviews Markus. Updated as per suggestion.

[marquiz]

Thanks for reviews Markus. Updated as per suggestion.

Thanks, looks quite simple and clean to me now 😎 Others might see it otherwisem though....

[marquiz]

/ok-to-test

[ArangoGutierrez]

/lgtm

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ArangoGutierrez, fmuyassarov, marquiz

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:

• OWNERS [marquiz]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[fmuyassarov]

/cherry-pick release-0.11

[k8s-infra-cherrypick-robot]

@fmuyassarov: #882 failed to apply on top of branch "release-0.11":

Applying: Run local markdown tests inside an isolated container
Using index info to reconstruct a base tree...
M	Makefile
Falling back to patching base and 3-way merge...
Auto-merging Makefile
CONFLICT (content): Merge conflict in Makefile
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Run local markdown tests inside an isolated container
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick release-0.11

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[fmuyassarov]

well, I will do it manually:)